Rely on Node builtinModules instead of maintaining a list manually

[vetruvet]

Currently webpack maintains a list of builtin Node.js modules that evolve over time. This PR changes that behavior to get the list of builtin modules from the Node.js module module.

The motivation for this was attempting to build a project that uses the diagnostics_channel polyfill package - webpack treats it as external even on Node<14.17, where it needs to be not externalized. It's possible to work around this in webpack's configuration by setting externalsPresets.node = false and then specifying node externals manually.

What kind of change does this PR introduce?

Refactoring and maintainability for future Node.js versions

Did you add tests for your changes?

I did not see any tests referencing the affected code (NodeTargetPlugin)

Does this PR introduce a breaking change?

If the target runtime is the same version of Node as what webpack is being run in, no.

Otherwise, there may be issues (e.g. if you're building on Node<14.17, but the resulting bundle will run on Node 16+, you would want diagnostics_channel to be externalized but it would not be). It's unclear to me if this is a supported use case, or if it's widely used anyway. Any input on this point is appreciated!

What needs to be documented once your changes are merged?

Webpack documentation does not have a comprehensive list of packages externalized by NodeTargetPlugin. If this is merged, the docs should be updated to specify that the list of externalized packages will be derived from require('module').builtinPlugins

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: vetruvet / name: Valera Trubachev (c116478)

[webpack-bot]

For maintainers only:

• This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
• This needs to be backported to webpack 4 (issue will be created when merged)

[TheLarkInn]

Does this mean that if Node pulls a built in away from that list, that it could break existing build behaviors?

[alexander-akait]

Yeah, it is a breaking change and will be unstable on different Node.js versions, anyway we can create a link task to parse them them from code and check with actual modules to catch missing things on CI

[TheLarkInn]

Yeah, it is a breaking change and will be unstable on different Node.js versions, anyway we can create a link task to parse them them from code and check with actual modules to catch missing things on CI

Agreed this is a much more stable approach.

@vetruvet would you be willing to work on this?

[vetruvet]

I see the point @TheLarkInn is making. I guess before going forward in any direction, I would like to understand consensus of what we want NodeTargetPlugin to actually do - specify latest Node.js builtins as externals regardless of what version webpack is running on? or specify the current Node.js builtins as externals?

If we're specifying latest Node builtins as externals, what is supposed to happen in the examples like diagnostics_channel? Or when you're building for Node<15 but webpack considers util/types an external when it doesn't actually exist?

Another can of worms that's opened up by this is that webpack allows you to specify which node version you're targeting with nodeX.Y as the target but it's not explicitly specified what node itself means - does it mean the latest Node version or the current? Regardless of which direction we go in, I think that should be clarified.

If possible/desired, we could also rely on the is-core-module package and pass it the explicit version specified in target. That package defaults to the current Node version if that makes any difference.

As for the CI route, my concern would be that since CI runs on multiple Node versions, different runs would produce different results. Presumably you would have to run this particular check on only the latest Node version, which goes back to the question at the beginning.

[alexander-akait]

@vetruvet

specify latest Node.js builtins as externals regardless of what version webpack is running on

This 👍

If we're specifying latest Node builtins as externals, what is supposed to happen in the examples like diagnostics_channel? Or when you're building for Node<15 but webpack considers util/types an external when it doesn't actually exist?

I think it is out of scope webpack, the same will be without webpack, you have to handle such cases on the application level using try/catch, webpack just bundle your code, not more

Another can of worms that's opened up by this is that webpack allows you to specify which node version you're targeting with nodeX.Y as the target but it's not explicitly specified what node itself means - does it mean the latest Node version or the current? Regardless of which direction we go in, I think that should be clarified.

If possible/desired, we could also rely on the is-core-module package and pass it the explicit version specified in target. That package defaults to the current Node version if that makes any difference.

I think here is the same.

But I think there's a potential improvement here that will satisfy both of us. We can use is-core-module (if it is small and don't have a lot of deps) and throw a warning on such cases (also we should have an option to enable/disable such behaviour, because it can be anoying), so you will see and catch CI fails

Feel free to feedback

[ShenHongFei]

Now the stream/consumers module is missing, causing me to get the following error when building

Module not found: Error: Can't resolve 'stream/consumers'

Glad to see this PR, hope it can be merged soon

[alexander-akait]

@ShenHongFei You can send a PR with stream/consumers, fix, because we are sitll have dicussion here, so if you need to fix soon (next release will be Wednesday) feel free to send a PR 👍

[ShenHongFei]

@alexander-akait Ok, I'm writing this PR. Let me first compare the differences between builtinModules in the latest node.js and the current one.

[ShenHongFei]

@vetruvet Is there still a need to add a wasi module, which is not currently in builtinModules?
https://nodejs.org/api/wasi.html

Suggested change

	"wasi",

[vetruvet]

I checked on Node.js 12, 14, 16, and 18, and they all have wasi included in builtinModules, even without the --experimental-wasi-unstable-preview1 flag.

[ShenHongFei]

I use Node.js 19.8.1

[vetruvet]

Ah, it seems in Node.js 19 (and moving forward, presumably), builtinModules only includes wasi if node is run with that flag... Which further pokes a hole in this approach if the goal is for this list to include all of the latest Node.js modules, regardless of the Node.js version/flags webpack itself is running under.

[ShenHongFei]

@alexander-akait Take a look at this PR #16841

[vetruvet]

@vetruvet

specify latest Node.js builtins as externals regardless of what version webpack is running on

This +1

Given that, we can't really use is-core-module (at least not in its current state), since that uses the current Node.js version if one is not specified, and it has no knowledge of what is the latest Node.js version. If we did use is-core-module as it is, webpack would have to know what the latest Node.js version is. While that is simpler than the list currently maintained, it's still manual toil. Thoughts?

[alexander-akait]

While that is simpler than the list currently maintained, it's still manual toil. Thoughts?

Yes, but we do it as part of a pre-release script, just move this list of modules to own file and create a script that will generate list of modules and throw an error if we don't make a release on the latest Node.js version, so we will always have the latest modules and it will not be manual work

[TheLarkInn]

@vetruvet I'm going to close this PR based on our discussions but would you be willing to contribute the creation of a pre-release script?

[vetruvet]

That works, thanks. I can't promise any timing but I will try to whip something up. If someone else can get to it sooner, more power to them!