-
Notifications
You must be signed in to change notification settings - Fork 49.8k
Stop treating all Node.js builtins implicitly as externals #34249
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
Conversation
83114a7 to
994ebab
Compare
|
Comparing: 698bb4d...d4f225f Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
1de4568 to
797f820
Compare
Sometimes only certain bundle types need them
797f820 to
d4f225f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks!
| // Use Node resolution mechanism. | ||
| resolve({ | ||
| // skip: externals, // TODO: options.skip was removed in @rollup/plugin-node-resolve 3.0.0 | ||
| preferBuiltins: bundle.preferBuiltins, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started this PR before bundle.preferBuiltins was added. That's also a valid API (but should probably be bundle.targetsNodeJS or something).
Noticed this when reviewing #34193.
We have an
externalsproperty for each bundle we build. This is supposed to prevent bundling modules that are listed in that entry.However, all Node.js modules were also treated as externals by
@rollup/plugin-node-resolve. This is wrong for bundles that are not targeting Node.js (e.g. Bun or edge environments). Instead, bundles need explicitly list the modules they support. For example, edge environments do support theutilmodule so they can list that. But edge environments don't supportstreamso it's not safe to mark that module as an external. It's not 100% accurate for modules that only have partial implementations. E.g.async_hooksis implemented in Bun and edge environments. But not all methods in that module (e.g.createHook) so you'd still have to be careful declaringasync_hooksas an external in Bun.I also removed some obvious unused externals from some bundles. We can't fail builds on unused externals since some bundle only use the external for a specific type e.g.
reactonly usesReactNativeInternalFeatureFlagsfor the RN bundles.Error with unexpected externals