-
Notifications
You must be signed in to change notification settings - Fork 49.3k
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!
@@ -402,8 +400,8 @@ function getPlugins( | |||
forbidFBJSImports(), | |||
// 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
externals
property 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 theutil
module so they can list that. But edge environments don't supportstream
so 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_hooks
is 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_hooks
as 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.
react
only usesReactNativeInternalFeatureFlags
for the RN bundles.Error with unexpected externals