pablo-abc / felte

An extensible form library for Svelte, Solid and React
https://felte.dev
MIT License
1.02k stars 44 forks source link

Solid reporter issue #97

Open davedbase opened 2 years ago

davedbase commented 2 years ago
Screen Shot 2022-01-30 at 4 08 59 PM

Ran into an issue with just importing reporter-solid:

import { reporter, ValidationMessage } from "@felte/reporter-solid";
pablo-abc commented 2 years ago

This seems to happen because Vite is grabbing the transpiled client code instead of the untranspiled one. And since the server version of solid-js/web does not export setAttribute it fails. I imagined the bundler would grab the JSX source and transpile it for the appropriate environment following this:

https://github.com/pablo-abc/felte/blob/5fb0611455feb649c94464c7ee6415941db6ebf2/packages/reporter-solid/package.json#L20-L26

But this does not seem to be happening in your case. I'll dig into this. Maybe providing an SSR transpiled version of the component and add it as a conditional export for Node could work? Although still, I imagine the ideal behaviour is for your bundler to transpile the component itself.

pablo-abc commented 2 years ago

I can't find anything that I might be doing different compared to other packages. Can you confirm if this happens with any other components? E.g. solid-app-router also uses setAttribute when transpiled. If so this might have more to do with Vite than this package.

pablo-abc commented 2 years ago

Hey, @davedbase. I had not tried to do SSR with Solid, but I used solid-ssr's examples as a template and I'm not getting this error. It makes me believe that this is indeed something in Vite. I'm not sure if it'd have something to do with this. I think this means Vite ignores the "solid" export by default?

Testing with SSR did show another issue when using SSR. Validation messages were being added at the end of the parent element instead of wherever <ValidationMessage> was being declared. I've pushed a fix for this on @felte/reporter-solid@1.0.0-next.22. Pushed the same change to 0.1.15 but that one does not seem to be working. I'll be checking on that later.


I've quickly cloned solid-ssr and modified its examples to use @felte/solid with @felte/reporter-solid which seems to be working.

millette commented 2 years ago

I'm using astro with solidjs and I'm also getting an error:

The requested module 'solid-js/web' does not provide an export named 'memo' file:///home/millette/waglo.com/node_modules/.pnpm/@felte+reporter-solid@1.0.0_solid-js@1.3.8/node_modules/@felte/reporter-solid/dist/index.js:1 import { template, setAttribute, memo } from 'solid-js/web';

SyntaxError: The requested module 'solid-js/web' does not provide an export named 'memo' at ModuleJob._instantiate (node:internal/modules/esm/module_job:127:21) at async ModuleJob.run (node:internal/modules/esm/module_job:193:5) at async Promise.all (index 0) at async ESMLoader.import (node:internal/modules/esm/loader:341:24) at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15) at async nodeImport (/home/millette/waglo.com/node_modules/.pnpm/vite@2.8.4/node_modules/vite/dist/node/chunks/dep-971d9e33.js:56246:21) at async eval (/src/components/Form.tsx:15:31) at async instantiateModule (/home/millette/waglo.com/node_modules/.pnpm/vite@2.8.4/node_modules/vite/dist/node/chunks/dep-971d9e33.js:56177:9)

I don't see at all where memo is coming from, I don't see it in the source, only in dist.js.

davedbase commented 2 years ago

@pablo-abc I installed @felte/reporter-solid@1.0.0-next.22 and get a similar issue to @millette with with the memo export issue.

pablo-abc commented 2 years ago

@millette @davedbase I managed to reproduce the issue using Astro. Adding this to vite's config solved the issue locally for me:

    ssr: {
      noExternal: ['@felte/reporter-solid'],
    },

If this is not set, Vite is completely ignoring the solid export on our package.json and imports the precompiled bundle from @felte/reporter-solid instead of the jsx one. Can either of you confirm if this also solves the issue on your side? (Seeing as both of you are using Vite). I'll add it as a note to the docs if this solves it.


As a side note: There are some issues when refreshing the page using Astro. This only happens during dev mode and get's solved when the module hot reloads? Seems to be an issue with Astro (or @astrojs/renderer-solid), though. After build it works without an issue.

millette commented 2 years ago

@pablo-abc Thanks, that fixed my issue. I haven't (yet) noticed anything weird in dev mode.

P.S.: I found the as prop on ValidationMessage was required otherwise I get this error:

Uncaught (in promise) TypeError: f is nul

pablo-abc commented 2 years ago

P.S.: I found the as prop on ValidationMessage was required otherwise I get this error:

While I did find some issues when using this in SSR (added a note on the documentation about it), the app does not crash for me when not using it, and I have not been able to get the error you're describing. The recommendation now would be to use the as prop for SSR to prevent any issues, but I'd rather the app not to crash if this is not used.

If you have a reproduction for this, it'd be nice if you open a new issue for this.