solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.05k stars 914 forks source link

Directive function is not defined #569

Closed lud closed 3 years ago

lud commented 3 years ago

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 commented 3 years ago

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.

lud commented 3 years ago

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 commented 3 years ago

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

lud commented 3 years ago

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

lud commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 :+1:

jfgodoy commented 3 years ago

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 commented 3 years ago

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: https://github.com/solidjs/vite-plugin-solid/issues/18

amoutonbrady commented 3 years ago

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

jfgodoy commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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.

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 commented 2 years ago

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 commented 2 years ago

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?