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

Enhancement: Improve CJS → ESM support #14998

Merged

Conversation

JakobJingleheimer
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer commented Dec 16, 2021

In order for Node.js to detect named exports in CJS, they must be statically available. However, without this change, webpack does not produce exports that way—webpack will respect source code that is originally written in such a way, but it won't itself produce it (ex if the source code is ESM).

The outputted code will now look like:

// …

function test() {} // user code

module.exports.test = test // this is the change

And when imported into an ESM file:

import * as cjs from './output.cjs';
import { test } from './output.cjs';

console.log(cjs);
/*
[Module: null prototype] {
  __esModule: true,
  default: { test: [Function: test] },
  test: [Function: test]
}
*/

console.log(test)l
/*
[Function: test]
*/

What kind of change does this PR introduce?

Enhancement

Did you add tests for your changes?

Will do once approach etc are confirmed.

Does this PR introduce a breaking change?

No: It requires the user to opt-in via configuration flag. Also, Node.js still automatically adds the named exports to default anyway (in addition to exposing them as named exports).

What needs to be documented once your changes are merged?

New output.library.type option commonjs-static which is similar to commonjs-module but emits code in a statically analyse-able way, so Node.js is able to construct named exports in it's commonjs esm compat layer.
Exports must be statically known to webpack for this to work. optimization.providedExports must not be disabled.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 16, 2021

CLA Signed

The committers are authorized under a signed CLA.

@webpack-bot
Copy link
Contributor

webpack-bot commented Dec 16, 2021

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)

@JakobJingleheimer

This comment has been minimized.

@JakobJingleheimer
Copy link
Contributor Author

@sokra requested/suggested module.exports not be hardcoded in the output, and to use __webpack_export_target__ instead, but doing so breaks named imports. So I don't know how to proceed.

fix externals

add test case
@sokra sokra marked this pull request as ready for review January 10, 2022 13:39
@JakobJingleheimer
Copy link
Contributor Author

Ohhh, now I see what you meant. Thanks!

I just ran it locally with the `${exportTarget}${nameAccess} = __webpack_exports__${exportAccess}${nameAccess};\n`, and it imports into an mjs file with named exports just fine using the exports.test = __webpack_exports__.test; commonjs-static outputted 🙂

@sokra sokra merged commit 149333f into webpack:main Jan 10, 2022
@sokra
Copy link
Member

sokra commented Jan 10, 2022

Thanks

@webpack-bot
Copy link
Contributor

I've created an issue to document this in webpack/webpack.js.org.

@@ -550,6 +550,7 @@ class ExternalModule extends Module {
case "commonjs":
case "commonjs2":
case "commonjs-module":
case "commonjs-static":
return getSourceForCommonJsExternal(request);
case "node-commonjs":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sokra what is this node-commonjs? I don't see it documented anywhere (just when it was added in #13622)

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.

None yet

3 participants