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: module-import get fallback from externalsPresets #18660

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

fi3ework
Copy link
Contributor

What kind of change does this PR introduce?

Adding fallback external type for "module-import" type when "module" and "import" are neither applicable.

In #18620, implement "module-import", but the premise is that we're using import or import() for external request, However, for example, if we use require('...'), "module-import" will failed to know which externals type to adopt ("import", "module", or even "node-commonjs"), and it will crash.

The possible options we may want to fall back to:

  1. node-commonjs: to let require('path') code could run in Node.js + ESM (this is widely used).
  2. import: possibly for some specific reason.
  3. module: possibly for some specific reason.
  4. commonjs: level the require there, to be bundled by a bundler.

In this PR, using externalsPresets to indicate which external to use as the fallback:

  1. when the exteranlsPresets is node.js related (electrons, node, nwjs): use node-commonjs
  2. when the exteranlsPresets is web: use module
  3. when the exteranlsPresets is webAsync: use import
  4. when exteranlsPresets is not assigned or derived, use commonjs

Since the implementation coupled the fallback with externalsPresets, when users want to set all ESM import/import() to module-import and leave other CJS require to node-commonjs, they need to override the node.js built-in externals type to module-import defined by externalsPresets.

There's a left TODO is when user setting multiple externalsPresets category, e.g. { node: true, web: true }. Which one should we choose to use as fallback. Currently using a simple prioritization module > import > node-commonjs.

Did you add tests for your changes?

Yes.

Does this PR introduce a breaking change?

No.

What needs to be documented once your changes are merged?

Update webpack/webpack.js.org#7345 after merged.

@webpack-bot
Copy link
Contributor

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)

@alexander-akait
Copy link
Member

Currently using a simple prioritization module > import > node-commonjs.

I am fine with it

@alexander-akait alexander-akait merged commit 62f4c68 into webpack:main Aug 14, 2024
55 of 57 checks passed
@alexander-akait
Copy link
Member

Thank you

@fi3ework
Copy link
Contributor Author

Thank you, I'll update the doc tonight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants