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

fix react import #1229

Merged
merged 11 commits into from
Apr 12, 2024
Merged

fix react import #1229

merged 11 commits into from
Apr 12, 2024

Conversation

mbostock
Copy link
Member

This took many hours. 😓 It’s important than import "react" works not just so that you can use React, but so you import React the same way whether you are running it in the browser or in Node. This allows us to use JSX components in server-side rendering (in Node) #1216, and then import the same JSX components in client-side rendering (in the browser) for hydration. (You can’t use npm:react in Node, at least not before we fix #288.)

What makes this difficult is that React is still distributed as CommonJS modules, and worse, React’s CommonJS modules are too dynamic to be understood by cjs-module-lexer, which is the de facto standard of how CommonJS modules are exposed as ES modules. As a result, when you consume React’s CommonJS as an ES module, you only get a default export instead of named exports. And that means import {useState} from "react" etc. doesn’t work.

I read a lot about various workarounds for this (such as rollup/plugins#423), but ultimately the only thing I got working was overriding the entry point into react (and react-dom and scheduler) to the production bundle which is compatible with cjs-module-lexer. But this is just luck — hopefully luck that holds until React ships ES modules. If this didn’t work, we’d have to do something like require the library and see what it exports at runtime, and then generate explicit named exports.

My overall sense is that importing from node_modules won’t work for many (especially older) packages — though it’ll certainly work for some. I expect better support for npm: imports because jsDelivr (and esm.sh and others) likely has many workarounds to make +esm work — and even then, as we’ve seen, it sometimes doesn’t. We should continue to advocate for package authors to ship ES modules. And we should promote standards-based alternatives such as JSR #956. Let’s not bake too many more workarounds into Framework… I mean, okay, if it’s easy. It wasn’t easy here.

@mbostock mbostock requested a review from Fil April 12, 2024 06:30
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Is there a simple-yet-exciting hello-world for react that we could use in lib/react.md?

@mbostock mbostock force-pushed the mbostock/fix-node-import-react branch from fa0920d to a3f838c Compare April 12, 2024 07:39
@mbostock
Copy link
Member Author

Sigh, so I discovered two more issues. First, we also need to patch the import for react/jsx-runtime. Second, the import of scheduler is still broken because the even the scheduler.production.min.js is not analyzable by cjs-module-lexer. So we’re going to need a more elaborate workaround for that, like a shim. 😞

@mbostock mbostock requested a review from Fil April 12, 2024 08:16
@mbostock
Copy link
Member Author

Fixed with a more robust shim, and added a hello-world example. React isn’t really usable without JSX, though.

Base automatically changed from mbostock/fix-node-import to main April 12, 2024 08:17
Comment on lines +118 to +120
external(source) {
return source.startsWith("/_node/");
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this shouldn’t necessary since our importResolve plugin already marks these as external: true, but the commonjs plugin causes issues without it.

esbuild({
format: "esm",
platform: "browser",
target: ["es2022", "chrome96", "firefox96", "safari16", "node18"],
exclude: [], // don’t exclude node_modules
define: {"process.env.NODE_ENV": JSON.stringify("production")},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in the future we could use development builds during preview here.

@Fil
Copy link
Contributor

Fil commented Apr 12, 2024

The tests seem flaky? I re-ran the one that failed and it passed.

@mbostock
Copy link
Member Author

Yes, I don’t know why.

@mbostock
Copy link
Member Author

The flakiness of the tests is because we run the tests in parallel, but we have a before hook that clears the cache:

await rm("docs/.observablehq/cache/_node", {recursive: true, force: true});

That means when a new test starts, it can delete the cache of a test that’s already running. Not sure why it doesn’t happen locally but we’ll need to do something else here to isolate the tests.

@mbostock mbostock merged commit e9b0856 into main Apr 12, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/fix-node-import-react branch April 12, 2024 15:45
@Fil Fil mentioned this pull request Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow npm: protocol imports from JavaScript & TypeScript data loaders
2 participants