solidjs / solid

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

`SVGElements` missing from jsx runtime #1483

Closed wooorm closed 1 year ago

wooorm commented 1 year ago

Describe the bug

Hey!

Important: this is from ESM (type: module), Node.js v19.3.0, npm 9.2.0, solid-js 1.6.9

import * as x from 'solid-js/h/jsx-dev-runtime'

console.log(x)
file:///.../node_modules/solid-js/h/dist/h.js:1
import { spread, assign, insert, createComponent, dynamicProperty, SVGElements } from 'solid-js/web';
                                                                   ^^^^^^^^^^^
SyntaxError: The requested module 'solid-js/web' does not provide an export named 'SVGElements'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

Node.js v19.3.0

Also with the production runtime:

import * as x from 'solid-js/h/jsx-runtime'

console.log(x)
file:///.../node_modules/solid-js/h/dist/h.js:1
import { spread, assign, insert, createComponent, dynamicProperty, SVGElements } from 'solid-js/web';
                                                                   ^^^^^^^^^^^
SyntaxError: The requested module 'solid-js/web' does not provide an export named 'SVGElements'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:124:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:190:5)

Node.js v19.3.0

Your Example Website or App

see above

Steps to Reproduce the Bug or Issue

see above

Expected behavior

Should import fine. Perhaps this is a bug with it missing in the ESM build. Or its a type only thing?

Screenshots or Videos

No response

Platform

Additional context

No response

ryansolid commented 1 year ago

Possible duplicate of #1480. Are you trying to use solid-js/h for server rendering?

wooorm commented 1 year ago

Oh darn, looks like a duplicate, sorry!

Server: Yeah, sort of, probably!

Writing a utility to turn hast (HTML syntax tree) into whatever through the automatic JSX runtime.

The goal is for folks to be able to use it on the client and on the server though.

wooorm commented 1 year ago

Is there are reason solid-js/h should not work with jsdom or so?

ryansolid commented 1 year ago

It can you just need to use the client build. You can use the browser export condition to do that easiest. Or the browser field in legacy tooling. We also have a plugin for Jest if that is what you are after.

Keep in mind that will generate HTML (in a more costly manner) but it won't be hydratable if that is the goal.

wooorm commented 1 year ago

Okay, so think this is an “expectations” issue on my side, but probably other people too.

This package, like other frameworks, exposes /jsx-runtime and /jsx-dev-runtime in its export map. Those names are used by the newish JSX automatic runtime. Unlike other frameworks, Solid doesn’t actually expose an automatic JSX runtime from those places. It exposes the entirety of Solid there: https://github.com/solidjs/solid/blob/deed4d6e272cb4d567d1c3b1e21f5f50b3f3740f/packages/solid/package.json#L97-L104 That doesn’t include jsx, jsxs, jsxDEV, or Fragment.

How the automatic runtime works is that folks configure either an option in their bundler to some package, or embed that with a comment. They’ll pass a string such as 'preact'. And then the bundler will import and access jsx/jsxs/jsxDEV/Fragment from preact/jsx-runtime or preact/jsx-dev-runtime. Because these export map entries exist, I guessed that I could pass solid-js too. That was incorrect. I have to pass solid-js/h instead (in browsers).

Second expectation is, when server side rendering, and assuming the above, I get a nasty error about SVGElements instead of something like “document doesn’t exist”.

To solve this, I recommend either:

But then there’s still the thing about how to use those runtimes in Node. Perhaps those can switch with a browser condition too? Or, if JSDom or so is needed, expose some code that triggers an error like “document doesn’t exist” instead?

ryansolid commented 1 year ago

We need the "./jsx-runtime" to support TS for our custom transform even if we don't actually use it. The custom transform is by far the recommended approach. And in so we set ourselves up so people can use our custom transform with TypeScript correctly out of the box. jsxImportSource solid-js + the option to preserve from TS works well. This approach works with client and server rendering and with hydration. We use export conditions to ensure the right version of the runtime get pulled in each case and it works well.

To complicate things more types for the HyperScript version are different than our custom transform. In so we do need to treat it as a separate thing. It is a different experience. So we can't remove or re-export. It would be nice to support SSR and hydration with this method in the future, to support situations where custom transform isn't viable but it would take some prioritizing. In this case you wouldn't get the SVG error because the server HyperScript runtime would expect the right stuff. I suppose the best thing we could do right now is detect if Solid things it is in server mode when setting up the HyperScript runtime and throwing with a message that suggests you set the "browser" condition.

wooorm commented 1 year ago

We need the "./jsx-runtime" to support TS for our custom transform even if we don't actually use it.

I don’t propose changing the types condition of /jsx-runtime: you use it to include the types for a JSX namespace, right? That’s fine. I’m proposing to change what’s now the default condition for /jsx-runtime and /jsx-dev-runtime. Because there is already working runtime code in h/jsx-runtime/dist/jsx.js, which can also be exposed from /jsx-runtime. And the current exposed code (dist/solid.js) isn’t an automatic JSX runtime, so that’s broken anyway.

I suppose the best thing we could do right now is detect if Solid things it is in server mode when setting up the HyperScript runtime and throwing with a message that suggests you set the "browser" condition.

Yes, redirecting on a node (or default) condition to a file that throws is an improvement over the current state!

Though, this is also close to the actual browser code, which throws an error if document is undefined? Then in Node for quick unit tests or so, one could use jsdom?