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

[BREAKING/v3] Exclude node builtins from build #1604

Merged
merged 1 commit into from
Apr 28, 2022

Conversation

cristiano-belloni
Copy link
Contributor

@cristiano-belloni cristiano-belloni commented Apr 21, 2022

TL;DR: exclude node builtins from build to make packages that import them but don't use them at runtime work. Works in esbuild mode and webpack mode, althought slightly differently.


Webpack v5 has stopped providing polyfills for imported Node builtins, as it's considered bad practice to import a "node" package in a "browser" app. Create React App is similarly opinionated, and doesn't provide any polyfill or fallback. I think modular should follow this opinion, unless we get overwhelming feedback indicating the contrary.

The problem is that some third party packages that provide both browser and node implementation don't distribute it into different files (there's a mechanism to do that, but it's not widely used yet), but they expose a single file that initially require()s everything (built-in modules included) and decides what to use at runtime, based - I guess - on capability sniffing (require(builtin); if (isNode) { useBuiltin() } else { useBrowseEquivalent() }). These modules, that would normally work without polyfills, now break the build. This happens, for example, with solclientjs and dotenv.

This PR adds fallback: false for all the node builtins required in the build, so that they get a non-meaningful value at runtime. If they're never used, that won't make a difference, but they won't break the build. This is breaking in respect to what modular v2 does (-> modular v2 uses webpack 4 that automatically provides polyfills)

This PR also externalises all node builtins in esbuild mode. This is analogous to what we do in Webpack mode, with the difference that if there is an npm module with the same name as a npm builtin in the extended dependencies, it will be excluded from the build (while webpack will use the homonym npm module instead). npm packages that are homonym to builtin packages are a legacy residue and are mainly deprecated, but if they're used by any third party library, they won't work in esbuild mode. I don't know a way to provide a fallback behaviour similar to webpack with esbuild. This is breaking in respect to what modular v2 in esbuild mode does (-> modular v2 always breaks when there is a builtin package in the build)

@changeset-bot
Copy link

changeset-bot bot commented Apr 21, 2022

🦋 Changeset detected

Latest commit: 327118a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
modular-scripts Major
eslint-config-modular-app Major
create-modular-react-app Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@LukeSheard
Copy link
Contributor

You can use node-builtins to generate this list, similar to what we already do with rollup.

Might be neater to construct this from that list rather than hard code it directly, this future proofs any new modules added in later node versions.

@cristiano-belloni
Copy link
Contributor Author

cristiano-belloni commented Apr 22, 2022

You can use node-builtins to generate this list, similar to what we already do with rollup.

Might be neater to construct this from that list rather than hard code it directly, this future proofs any new modules added in later node versions.

Yeah, that makes sense. I was a bit nervous using an unofficial list that someone has to update and is one level away from us, but it seems half on the Internet rests on Sindre's shoulders anyway :)
Fixed to use node-builtins.

@cristiano-belloni cristiano-belloni removed the request for review from LukeSheard April 27, 2022 14:40
steveukx
steveukx previously approved these changes Apr 28, 2022
@LukeSheard LukeSheard changed the title Exclude node builtins from build [BREAKING/v3] Exclude node builtins from build Apr 28, 2022
@LukeSheard LukeSheard force-pushed the feature/native-node-modules-fix branch from e01a1e1 to 327118a Compare April 28, 2022 13:56
@LukeSheard LukeSheard changed the base branch from release/v3 to main April 28, 2022 13:56
@LukeSheard LukeSheard dismissed steveukx’s stale review April 28, 2022 13:56

The base branch was changed.

@LukeSheard LukeSheard merged commit d7c8725 into main Apr 28, 2022
@LukeSheard LukeSheard deleted the feature/native-node-modules-fix branch April 28, 2022 13:57
This was referenced Apr 28, 2022
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.

3 participants