Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Directive function is not defined #569

Closed
lud opened this issue Jul 18, 2021 · 24 comments
Closed

Directive function is not defined #569

lud opened this issue Jul 18, 2021 · 24 comments
Labels
question Further information is requested typescript relating to typescript or types

Comments

@lud
Copy link

lud commented Jul 18, 2021

Hello,

I used the vite-solid-tailwind-starter package to start a project and integrate the app with Phoenix Framework.

The starter features a dropdown so I decided to integrate the clickOutside directive from the tutorial.

I added this bit to my div:

use:clickOutside={() => setShowProfileMenu(false)}

First it has this error:

Type '{ children: Element; "use:clickOutside": () => boolean; class: string; }' is not assignable to type 'HTMLAttributes<HTMLDivElement>'.
  Property 'use:clickOutside' does not exist on type 'HTMLAttributes<HTMLDivElement>'.ts(2322)

But as the tutorial features a //@ts-nocheck comment I guess it is a known bug.

Now, when I run the app, I have an error:

Uncaught ReferenceError: clickOutside is not defined

If I add a dummy console.log(clickOutside) anywhere in the file then the directive works as expected.

It looks like the tree is shaken a bit too much.

I get the same behaviour with the default code in the starter (without configuring for Phoenix).

I am not sure if I am missing something, as it should work, since it works in the tutorial.

Thank you

@ryansolid
Copy link
Member

Yeah there is two things going on.
Firstly Directives need to be extended on to the JSX namespace. I didn't put this in the tutorial since it's a bit verbose and I didn't want to distract. But based on this

declare module "solid-js" {
  namespace JSX {
    interface Directives {
      use:clickOutside: boolean;
    }
  }
}

The second part, which I've only realized recently, is TS likes to strip out unused variables even before the bundler gets its hands on it. You can tell it to only strip out unused types with this option:

{ onlyRemoveTypeImports: true }

With both of these you should be set.

@ryansolid ryansolid added question Further information is requested typescript relating to typescript or types labels Jul 19, 2021
@lud
Copy link
Author

lud commented Jul 20, 2021

Hi @ryansolid , Thank you for your answer.

Using your snippet with quoted property name:

declare module "solid-js" {
  namespace JSX {
    interface Directives {
      'use:clickOutside': boolean;
    }
  }
}

I tried to add the module declaration in declaration.d.ts got new problems, so I moved it to a src/custom-types/custom.d.ts file and added fthe following to my tsconfig:

"typeRoots": [
      "./src/custom-types",
      "./node_modules/@types",
    ]

The problem is that now I have errors everywhere : Module '"solid-js"' has no exported member 'Component', as if my declaration was overwriting the whole solid-js module.

Maybe that is my vscode setup that is wrong.

This I do not know where to add:

{ onlyRemoveTypeImports: true }

Searching on google it seems it is a babel setting, though I use Vite.

I will try to make a minimal project to try with a clean start.

@lud
Copy link
Author

lud commented Jul 20, 2021

Also if my directive is a .tsx and not a .ts then the passed accessor will be undefined.

@lud
Copy link
Author

lud commented Jul 20, 2021

I used the tutorial code in a demo, if you want to take a look: https://github.com/lud/-click-outside

@lud
Copy link
Author

lud commented Jul 20, 2021

Now using a snowpack boilerplate and it works without the console.log thanks to the config flag you gave me as it uses babel.

I've also found a workaround for the type declarations:

declare module 'solid-js' {
  namespace JSX {
    interface HTMLAttributes<T> {
      'use:clickOutside'?: () => unknown
    }
  }
}

Though it is not following the solid js docs.

@ryansolid
Copy link
Member

Yeah the babel transform in Vite probably needs to be handled by vite-plugin-solid which uses babel under the hood to transform Solid's .tsx files. Although not sure why Directives interface wouldn't work since HTMLAttributes inherit from them. I use that pattern in Solid's tests and I used it in our previous starter. But I am seeing what you are describing in the playground. So I suspect it isn't limited to just there.

@ryansolid
Copy link
Member

Oh sorry the docs are bad. Or at least misleading. It should be:

declare module "solid-js" {
  namespace JSX {
    interface Directives {
      clickOutside?: () => void;
    }
  }
}

You don't need to put the namespace in. It automatically applies it.

@lud
Copy link
Author

lud commented Jul 23, 2021

Right! That works.
I've also finally found how to configure with Vite:

import { defineConfig } from "vite";
import solidPlugin from "vite-plugin-solid";

export default defineConfig({
  plugins: [solidPlugin({ babel: { onlyRemoveTypeImports: true } })],
  // ...
})  

Also the declaration works if it is right above the code:

import { onCleanup } from "solid-js";

declare module "solid-js" {
  namespace JSX {
    interface Directives {
      clickOutside?: () => void;
    }
  }
}

export default function clickOutside(el, accessor) {
  const onClick = (e) => !el.contains(e.target) && accessor()?.();
  document.body.addEventListener("click", onClick);

  onCleanup(() => document.body.removeEventListener("click", onClick));
}

"go to definition" shortcuts on code editors would then directly go to the right file.

Thank you for your help 👍

@lud lud closed this as completed Jul 23, 2021
@jfgodoy
Copy link

jfgodoy commented Aug 23, 2021

Hi, I having the same issue.

I couldn't configure vite. When I add the option onlyRemoveTypeImports I get the following error:

[vite] Internal server error: Unknown option: .onlyRemoveTypeImports. Check out https://babeljs.io/docs/en/babel-core/#options for more information about options.
  Plugin: solid

do I missing something?

For now I figured out a workaround

// file solid-extensions.tsx
import "solid-js";
declare module "solid-js" {
  namespace JSX {
    interface Directives {
      "autoresize": boolean
    }
  }
}

export function autoresize(el: HTMLInputElement) {
  let old_value = el.value;
  el.addEventListener("input", () => {
    if (old_value.length > el.value.length) {
      el.style.width = "0";
    }
    el.style.width = `${el.scrollWidth}px`
    old_value = el.value;
  });
}

export function useDirective(_directive: any) {
  /* this function does nothing. It's just a workaround to avoid
  babel tree shake our directive before solidjs has the oportunity to use it
  */
}
import {autoresize, useDirective} from "./solid-extensions";

useDirective(autoresize);

@ryansolid
Copy link
Member

I think it's actually on babel-preset-typescript which means we'd need to change the plugin to set that option on the TS conversion. Given how this breaks directives probably should just have it configured that way by default. Made an issue: solidjs/vite-plugin-solid#18

@amoutonbrady
Copy link
Member

This is fixed in vite-plugin-solid@2.0.2, give it a try and let me know :)

@jfgodoy
Copy link

jfgodoy commented Aug 28, 2021

I couldn't test the directives. The change onlyRemoveTypeImports: true introduced in vite-plugin-solid@2.0.2 broke my project in all places where I had import { SomeType } from "./a/path" instead of import type { SomeType } from "./a/path".

@ryansolid
Copy link
Member

Yeah I had to go in and fix up the templates. But as far as we know that's the only way to get TS to not strip out unused imports. It really shouldn't be its responsibility but it does that. So far we haven't found another way to get TS to play nice and not pre-emptively strip it.

@amoutonbrady
Copy link
Member

Thank you Ryan for doing that. I published a new version this morning that that reverted the change and let you manually opt-in the option.

Sorry for that one :/

@codechips
Copy link

Hi! I've also stumbled upon this same issue recently, but solved it with the help above. Thanks to all of you! However, I now get a different error with the click outside function above.

Uncaught TypeError: accessor is not a function

My directive looks like this: use:clickOutside={() => setShowMenu(false)}, but somehow the passed in closure ends up being undefined in the body of the directive function. The click outside directive function is the "official" one from the tutorial. Any ideas on where the problem might be?

@ryansolid
Copy link
Member

@codechips Not sure. Looking at the tutorial as well as the latest compiled output the example looks fine. Need to see what it compiles to. Can you reproduce on https://playground.solidjs.com or a codesandbox etc..?

@codechips
Copy link

@ryansolid I tried this in playground and it works fine. I think it's because of how the code is compiled there. However, I've managed to reproduce it with the setup I have in my project. Here is the repo https://github.com/codechips/solidjs-clickoutside-bug

Both versions, working and broken, compile down to this for the directive.

 clickOutside(_el$3, () => () => setShowMenu(false));

It might be that it's something very obvious, but I am totally code-blind atm 😅

@jfgodoy
Copy link

jfgodoy commented Sep 2, 2021

I have fallen in the same trap before... when you export a default function from a .tsx is considered a Component. You have two options, use a named export or change the extension of click-outside.tsx to .ts

@ryansolid
Copy link
Member

@jfgodoy thanks. We need to work on that heuristic a bit. It's a bit tricky though since functions don't tend to have much identifying with them. I guess we could see if it only has one or no arguments. In this case these tend to have 2. But it wouldn't be fullproof as boolean directives might not use the second argument. Short of doing some sort of cross template analysis this might be tough.

@codechips
Copy link

Thanks a ton @jfgodoy! Renaming to .ts helped. After all it makes sense. It's not a component, but a utility function. Why name it .tsx? Use directives are awesome. I love them in Svelte and glad that they exist in Solid too.

@ryansolid maybe the directive file should be renamed in the tutorial and examples as well in order to avoid the confusion. If not, maybe add a note in the docs on when to use .tsx and when you can get away with just .ts or .js. However, I don't know if this issue is bundler specific or not.

@ryansolid
Copy link
Member

It's specific to the hot reload plugin. So yeah we should look at the file times. By default the playground(tutorials) only offers up .tsx and .css. I think we should expand that.

@evertheylen
Copy link

In my rather simple vite project, none of the proposed solutions seems to work. I have a simple anchorMenu directive that a applies an extra class to an element (see MDC), but I can't just use/import it.

  • defining the directive in a .ts file -> anchorMenu is not defined
  • using solidPlugin({ babel: { onlyRemoveTypeImports: true } }) -> [vite] Internal server error: Unknown option: .onlyRemoveTypeImports. I don't know where this option went? I searched online but no luck.

Currently I just "use" it as a sole statement before I use it as a directive:

anchorMenu;
//...
<div use:anchorMenu>

This works, but it is obviously rather clunky to use. Any tips? I can create a full demo if you want, sadly it does work in the playground.

@otonashixav
Copy link
Contributor

The option is here now; it should be solidPlugin({ typescript: { onlyRemoveTypeImports: true } }).

Note that you have to both declare the directive and add the plugin option, not just one or the other. The first prevents typescript from complaining that the directive doesn't exist in props, and the second prevents typescript from removing the import during compilation as it assumes you didn't use it anywhere (but Solid's transform will, resulting in a runtime error).

@rosostolato
Copy link

It should have a way to let typescript reference the function when using use: keyword as it does for the Component function. Have someone raised an issue on their github?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested typescript relating to typescript or types
Projects
None yet
Development

No branches or pull requests

8 participants