[commonjs] Add `defaultIsModuleExports` option to match Node.js behavior #838

nicolo-ribaudo · 2021-03-18T11:35:51Z

Rollup Plugin Name: `commonjs`

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

List any relevant issue numbers: Fixes #635

Description

I was trying to modify Babel's sources (ref) to that they work:

When compiled to CJS with Babel
When compiled to ESM in Node.js
When bundled with Rollup

I achieved (2) by authoring our sources according to what Node.js expects.
I achieved (1) using a small custom Babel plugin, and we'll make it an option (babel/babel#12838) of @babel/plugin-transform-modules-commonjs in the next minor version.
I didn't realize (3) was not working, I now opened a PR with another custom Babel plugin (babel/babel#13017) which replaces import foo with import * as foo for the few CJS deps we have that use __esModule = true.

~~I think that this option should eventually default to true in the future, but we are probably not ready for it yet.~~

lukastaegert · 2021-03-18T13:52:07Z

packages/commonjs/README.md

@@ -174,6 +174,13 @@ If you set `esmExternals` to `true`, this plugins assumes that all external depe

 You can also supply an array of ids to be treated as ES modules, or a function that will be passed each external id to determine if it is an ES module.

+### `nodeDefaultImport`


Thanks for moving forward here!

I was thinking about the name of the option and I am not happy about this choice as it references an environment (Node) that may change while IMO it should rather reflect functionality. So what this is about is as I understand it is to skip the default import interop in favour of assuming the default is module.exports. There are several ways to handle this (names are just suggestions):

moduleExportsIsDefault: true | false | "auto" where "auto" is the current behaviour (could also be "detect")

cjsDefaultExport: "module.exports" | "default" | "auto" where "auto" is the current behaviour (or cjsDefaultImport depending which way you view it)

Admittedly that the version where the default export is always exports.default may not be as useful, but what do I know. Could be a micro-optimisation for some. Thoughts?

Not sure we should change the default, but I keep twisting the idea of a config plugin in my head that would just configure commonjs, node-resolve and rollup for Node compatibility.

I have a slight preference for the moduleExportsIsDefault: true | false | "auto" (or defaultIsModuleExports? since we are giving a value to the "default" binding) option, since typing "module.exports" inside a string (thus often without autocompletion) can be a bit harder.

There are already precedents for boolean | string options, such as requireReturnsDefault.

Admittedly that the version where the default export is always exports.default may not be as useful, but what do I know. Could be a micro-optimisation for some. Thoughts?

We actually have an option in Babel exactly for that (https://babeljs.io/docs/en/babel-plugin-transform-modules-commonjs#nointerop), even if we don't have data about how many people use it.

But you do not have the false case, i.e. always assuming the default export is module.exports.default, right? That's the one where I am not sure people really would use it (and if we should even add it).

noInterop: true means "always get .default, without first checking if it has __esModule: https://github.com/babel/babel/tree/main/packages/babel-plugin-transform-modules-commonjs/test/fixtures/noInterop/import-default-only

Ah I see. So reading babel/babel#12838 I understand now that you are also going for a three-state flag 👍

or defaultIsModuleExports

I would be fine with that as well.

nicolo-ribaudo · 2021-03-19T11:37:28Z

packages/commonjs/test/fixtures/form/defaultIsModuleExports-false-no-default/output.js

+	named: named
+};
+
+export default input.default;


I'm not sure about this one. Should we avoid injecting the default export when it's not present? But then, how do we handle cases where input.default exists but it's not easily statically analyzable?

Good question. I would lean towards adding it always for now, though maybe as undefined, as this is similar to the current behaviour.

lukastaegert

Thanks for putting in the work!

guybedford · 2021-04-03T15:34:25Z

Great work, thanks @nicolo-ribaudo for the PR.

nicolo-ribaudo · 2021-04-03T15:34:46Z

Thanks @lukastaegert and @guybedford for the reviews!

nicolo-ribaudo · 2021-04-23T22:11:43Z

We are releasing Babel 7.14.0 next week, and it contains an option which is almost the same as this one.

Is it ok if we mention in the release post that the next version of @rollup/plugin-commonjs will support this parallel option?

guybedford · 2021-04-26T15:16:43Z

@nicolo-ribaudo thanks for the heads up, I'll do my best to see if we can be release ready by the end of the week but can't guarantee it.

…behavior (#838) * [commonjs] Add `nodeDefaultImport` option to match Node.js behavior * Undo unrelated changes * Undo editor autoformatter * Change option

jedwards1211 · 2023-11-02T17:19:48Z

@lukastaegert @nicolo-ribaudo is there not a way to get @rollup/plugin-babel to just output interopRequireDefault calls on external modules like my standalone babel transpilation does?

We still have a lot of external modules that are transpiled CJS with exports.default declarations, and you can probably imagine how painful it would be to try to update all of those packages to either be dual ESM/CJS or have a CJS-style module.exports = default export along with named exports.

jedwards1211 · 2023-11-02T17:47:23Z

Okay nevermind, seems like I can get it to work by using

    getBabelOutputPlugin({
      plugins: ['@babel/plugin-transform-modules-commonjs'],
    }),

nicolo-ribaudo · 2023-11-02T17:50:15Z

Can you try using interop: auto instead (https://rollupjs.org/configuration-options/#output-interop)? Rollup works better when it receives ESM input, rather than when Babel compiles ESM to CJS for it.

jedwards1211 · 2023-11-02T17:57:29Z

@nicolo-ribaudo oh, many thanks! Sorry, I hadn't discovered that option.

nicolo-ribaudo added 2 commits March 18, 2021 12:22

[commonjs] Add nodeDefaultImport option to match Node.js behavior

1d788e0

Undo unrelated changes

Verified

This commit was signed with the committer’s verified signature.

jeevatkm Jeevanandam M.

GPG key ID: 15B44CCA54B0059C

Verified
Learn about vigilant mode

773a457

This was referenced Mar 18, 2021

CommonJS Node.js Default Interop Case #635

Closed

Fix importing polyfill plugins in the Rollup bundle babel/babel#13017

Merged

Undo editor autoformatter

599c8b9

lukastaegert reviewed Mar 18, 2021

View reviewed changes

nicolo-ribaudo force-pushed the commonjs-node-interop branch from b514add to 27c5bdc Compare March 19, 2021 11:34

Change option

fc73572

nicolo-ribaudo force-pushed the commonjs-node-interop branch from 27c5bdc to fc73572 Compare March 19, 2021 11:35

nicolo-ribaudo commented Mar 19, 2021

View reviewed changes

nicolo-ribaudo changed the title ~~[commonjs] Add nodeDefaultImport option to match Node.js behavior~~ [commonjs] Add defaultIsModuleExports option to match Node.js behavior Mar 19, 2021

nicolo-ribaudo mentioned this pull request Mar 19, 2021

Provide an ESM build babel/babel-polyfills#75

Merged

lukastaegert self-requested a review March 20, 2021 05:54

lukastaegert approved these changes Mar 20, 2021

View reviewed changes

fwh1990 mentioned this pull request Mar 22, 2021

Legacy build fails on babel step vitejs/vite#2442

Closed

guybedford approved these changes Apr 3, 2021

View reviewed changes

guybedford merged commit 53776ee into rollup:master Apr 3, 2021

nicolo-ribaudo deleted the commonjs-node-interop branch April 3, 2021 15:34

pfdgithub mentioned this pull request May 23, 2023

Invalid define value (must be an entity name or valid JSON syntax): {"BASE_URL":"./","MODE":"production","DEV":false,"PROD":true,"LEGACY":__VITE_IS_LEGACY__} pengzhanbo/vite-plugin-mock-dev-server#31

Closed

0xdevalias mentioned this pull request Dec 6, 2023

update rollup from 2.35.1 (2020-12-14) to at least 3.16.0 (2023-02-17), or ideally latest (4.6.1, 2023-11-30) developit/microbundle#1067

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[commonjs] Add `defaultIsModuleExports` option to match Node.js behavior #838

[commonjs] Add `defaultIsModuleExports` option to match Node.js behavior #838

nicolo-ribaudo commented Mar 18, 2021 •

edited

Loading

lukastaegert Mar 18, 2021

lukastaegert Mar 18, 2021

nicolo-ribaudo Mar 18, 2021 •

edited

Loading

lukastaegert Mar 18, 2021

nicolo-ribaudo Mar 18, 2021

lukastaegert Mar 18, 2021

nicolo-ribaudo Mar 19, 2021

lukastaegert Mar 20, 2021

lukastaegert left a comment

guybedford commented Apr 3, 2021

nicolo-ribaudo commented Apr 3, 2021

nicolo-ribaudo commented Apr 23, 2021 •

edited

Loading

guybedford commented Apr 26, 2021

jedwards1211 commented Nov 2, 2023 •

edited

Loading

jedwards1211 commented Nov 2, 2023

nicolo-ribaudo commented Nov 2, 2023

jedwards1211 commented Nov 2, 2023

		@@ -174,6 +174,13 @@ If you set `esmExternals` to `true`, this plugins assumes that all external depe

		You can also supply an array of ids to be treated as ES modules, or a function that will be passed each external id to determine if it is an ES module.

		### `nodeDefaultImport`

[commonjs] Add defaultIsModuleExports option to match Node.js behavior #838

[commonjs] Add defaultIsModuleExports option to match Node.js behavior #838

Conversation

nicolo-ribaudo commented Mar 18, 2021 • edited Loading

Rollup Plugin Name: commonjs

Description

lukastaegert Mar 18, 2021

Choose a reason for hiding this comment

lukastaegert Mar 18, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

lukastaegert Mar 18, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Mar 18, 2021

Choose a reason for hiding this comment

lukastaegert Mar 18, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Mar 19, 2021

Choose a reason for hiding this comment

lukastaegert Mar 20, 2021

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

guybedford commented Apr 3, 2021

nicolo-ribaudo commented Apr 3, 2021

nicolo-ribaudo commented Apr 23, 2021 • edited Loading

guybedford commented Apr 26, 2021

jedwards1211 commented Nov 2, 2023 • edited Loading

jedwards1211 commented Nov 2, 2023

nicolo-ribaudo commented Nov 2, 2023

jedwards1211 commented Nov 2, 2023

[commonjs] Add `defaultIsModuleExports` option to match Node.js behavior #838

[commonjs] Add `defaultIsModuleExports` option to match Node.js behavior #838

nicolo-ribaudo commented Mar 18, 2021 •

edited

Loading

Rollup Plugin Name: `commonjs`

nicolo-ribaudo Mar 18, 2021 •

edited

Loading

nicolo-ribaudo commented Apr 23, 2021 •

edited

Loading

jedwards1211 commented Nov 2, 2023 •

edited

Loading