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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from Apr 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d788e0
[commonjs] Add `nodeDefaultImport` option to match Node.js behavior
nicolo-ribaudo Mar 18, 2021
773a457
Undo unrelated changes
nicolo-ribaudo Mar 18, 2021
599c8b9
Undo editor autoformatter
nicolo-ribaudo Mar 18, 2021
fc73572
Change option
nicolo-ribaudo Mar 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/commonjs/README.md
View file
Open in desktop
Expand Up @@ -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`
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


Type: `boolean`<br>
Default: `false`

When `true`, match the Node.js behavior when importing the `default` import from a CommonJS module, making it return the value of `module.exports`.

### `requireReturnsDefault`

Type: `boolean | "namespace" | "auto" | "preferred" | ((id: string) => boolean | "auto" | "preferred")`<br>
Expand Down
6 changes: 4 additions & 2 deletions packages/commonjs/src/generate-exports.js
View file
Open in desktop
Expand Up @@ -20,7 +20,8 @@ export function rewriteExportsAndGetExportsBlock(
isRestorableCompiledEsm,
code,
uses,
HELPERS_NAME
HELPERS_NAME,
nodeDefaultImport
) {
const namedExportDeclarations = [`export { ${moduleName} as __moduleExports };`];
const moduleExportsPropertyAssignments = [];
Expand Down Expand Up @@ -79,7 +80,8 @@ export function rewriteExportsAndGetExportsBlock(
defaultExport.push(`export default ${deconflictedDefaultExportName || moduleName};`);
} else if (
(wrapped || deconflictedDefaultExportName) &&
(defineCompiledEsmExpressions.length > 0 || code.indexOf('__esModule') >= 0)
(defineCompiledEsmExpressions.length > 0 ||
(!nodeDefaultImport && code.indexOf('__esModule') >= 0))
) {
// eslint-disable-next-line no-param-reassign
uses.commonjsHelpers = true;
Expand Down
3 changes: 2 additions & 1 deletion packages/commonjs/src/index.js
View file
Open in desktop
Expand Up @@ -145,7 +145,8 @@ export default function commonjs(options = {}) {
dynamicRequireModuleSet,
disableWrap,
commonDir,
ast
ast,
options.nodeDefaultImport
);
}

Expand Down
8 changes: 5 additions & 3 deletions packages/commonjs/src/transform-commonjs.js
View file
Open in desktop
Expand Up @@ -53,7 +53,8 @@ export default function transformCommonjs(
dynamicRequireModuleSet,
disableWrap,
commonDir,
astCache
astCache,
nodeDefaultImport
) {
const ast = astCache || tryParse(parse, code, id);
const magicString = new MagicString(code);
Expand Down Expand Up @@ -137,7 +138,7 @@ export default function transformCommonjs(
// we're dealing with `module.exports = ...` or `[module.]exports.foo = ...` –
if (programDepth > 3) {
shouldWrap = true;
} else if (exportName === KEY_COMPILED_ESM) {
} else if (!nodeDefaultImport && exportName === KEY_COMPILED_ESM) {
defineCompiledEsmExpressions.push(parent);
} else if (flattened.keypath === 'module.exports') {
topLevelModuleExportsAssignments.push(node);
Expand Down Expand Up @@ -470,7 +471,8 @@ export default function transformCommonjs(
isRestorableCompiledEsm,
code,
uses,
HELPERS_NAME
HELPERS_NAME,
nodeDefaultImport
);

const importBlock = rewriteRequireExpressionsAndGetImportBlock(
Expand Down
5 changes: 5 additions & 0 deletions packages/commonjs/test/fixtures/form/nodeDefaultImport-__esModule/_config.js
View file
Open in desktop
@@ -0,0 +1,5 @@
module.exports = {
options: {
nodeDefaultImport: true
}
};
3 changes: 3 additions & 0 deletions packages/commonjs/test/fixtures/form/nodeDefaultImport-__esModule/input.js
View file
Open in desktop
@@ -0,0 +1,3 @@
exports.__esModule = true;
exports.default = 2;
exports.named = 3;
14 changes: 14 additions & 0 deletions packages/commonjs/test/fixtures/form/nodeDefaultImport-__esModule/output.js
View file
Open in desktop
@@ -0,0 +1,14 @@
var __esModule = true;
var _default = 2;
var named = 3;

var input = {
__esModule: __esModule,
default: _default,
named: named
};

export default input;
export { input as __moduleExports };
export { __esModule };
export { named };
5 changes: 5 additions & 0 deletions packages/commonjs/test/fixtures/form/nodeDefaultImport-no-__esModule/_config.js
View file
Open in desktop
@@ -0,0 +1,5 @@
module.exports = {
options: {
nodeDefaultImport: true
}
};
2 changes: 2 additions & 0 deletions packages/commonjs/test/fixtures/form/nodeDefaultImport-no-__esModule/input.js
View file
Open in desktop
@@ -0,0 +1,2 @@
exports.default = 2;
exports.named = 3;
11 changes: 11 additions & 0 deletions packages/commonjs/test/fixtures/form/nodeDefaultImport-no-__esModule/output.js
View file
Open in desktop
@@ -0,0 +1,11 @@
var _default = 2;
var named = 3;

var input = {
default: _default,
named: named
};

export default input;
export { input as __moduleExports };
export { named };