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

esm: add option to interpret __esModule like Babel #40892

Closed
wants to merge 3 commits into from

Conversation

qnighy
Copy link

@qnighy qnighy commented Nov 20, 2021

This PR adds an option --cjs-import-interop. When enabled, a CJS module will be translated to ESM slightly differently with respect to default exports.

  • When the module defines module.exports.__esModule as a truthy value, the value of module.exports.default is used as the default export.
  • Otherwise, module.exports is used as the default export. (existing behavior)

It allows better interoperation between full ES modules and CJS modules transformed from ES modules by Babel or tsc. Consider the following example:

// Transformed from:
// export default "Hello";
Object.defineProperty(module.exports, "__esModule", { value: true });
module.exports.default = "Hello";

When imported from the following module:

import greeting from "./hello.cjs";
console.log(greeting);

With --cjs-import-interop, it will print "Hello".

Fixes: #40891

Adds an option `--cjs-import-interop`. When enabled, a CJS module will
be translated to ESM slightly differently with respect to default
exports.

- When the module defines `module.exports.__esModule` as a truthy value,
  the value of `module.exports.default` is used as the default export.
- Otherwise, `module.exports` is used as the default export.
  (existing behavior)

It allows better interoperation between full ES modules and CJS modules
transformed from ES modules by Babel or tsc. Consider the following
example:

```javascript
// Transformed from:
// export default "Hello";
Object.defineProperty(module.exports, "__esModule", { value: true });
module.exports.default = "Hello";
```

When imported from the following module:

```javascript
import greeting from "./hello.cjs";
console.log(greeting);
```

With `--cjs-import-interop`, it will print "Hello".

Fixes: nodejs#40891
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Nov 20, 2021
src/node_options.cc Outdated Show resolved Hide resolved
Co-authored-by: Voltrex <mohammadkeyvanzade94@gmail.com>
@mscdex
Copy link
Contributor

mscdex commented Nov 20, 2021

/cc @nodejs/modules

@bmeck
Copy link
Member

bmeck commented Nov 20, 2021

I'm not a fan unfortunately. We had discussed this in the past at length @guybedford and I. In particular I don't think this should be a global flag. Needing to have continuous support for a global flag is honestly much more a burden than doing a per package or per module option to configure this. I personally think the evaluation prior to linkage causes other issues that don't act 100% like babel. Even with __esModule various tools that do support it do not act 100% the same: https://sokra.github.io/interop-test/#default-export . With the current mode of operation, module.exports is always available even if it is a function so you can still use the function even if it doesn't create a circular property and that was the original thinking. Allowing authors of modules to opt out of this seems fine, or even allowing an individual consumer to opt out of this seems fine, but forcing all module code to act differently without per module exception seems to just add burden. If the intent is to always enable this for example, I don't want it to be a CLI flag.

@qnighy
Copy link
Author

qnighy commented Nov 20, 2021

Thank you for the detailed feedback!

We had discussed this in the past at length @guybedford and I.

Is the discussion in a written form and public? If so, I'd like to look through the discussion for better design. I thought someone must have already considered the problem but I couldn't find relevant discussions.

Allowing authors of modules to opt out of this seems fine, or even allowing an individual consumer to opt out of this seems fine, but forcing all module code to act differently without per module exception seems to just add burden.

I see. I'll try to design per-module options then. With that in mind, is it acceptable to keep the CLI option to determine the default? The following options came to my mind:

  • The new behavior is opt-in in a per-module manner.
  • The new behavior becomes the default in the latest and the next major version. It is opt-in in a per-module manner when backported to the stable versions.
  • The CLI option is kept and is used to determine the default.

@bmeck
Copy link
Member

bmeck commented Nov 20, 2021

With that in mind, is it acceptable to keep the CLI option to determine the default?

I'd say it is unlikely and probably could only be used for testing purposes. See examples of --es-module-specifier-resolution lacking maintenance and not having path to becoming the default, --preserve-symlinks lacking maintenance and general usage even by package managers using symlinks, and a flag for configuring package.json's default type: #32394 . The problem with global flags is that it means you get into cases where a module only works with the global flag and effectively splits node into 2 entirely different modes of operations. Additionally they don't work well with hashbang style execution since #!/... doesn't always accept multiple arguments.

In recent times @GeoffreyBooth and I have thought to remove --es-module-specifier-resolution as it isn't maintained in part because it is a mode of operation outside of the normal way of using node.

The new behavior becomes the default in the latest and the next major version. It is opt-in in a per-module manner when backported to the stable versions.

I do not want this to happen. Absolutely not going to become the default. Becoming the default would mean a variety of CJS modules cannot use their full module.exports and is a breaking change so that all code written for the current default would need to be rewritten. It would need an incredible amount of work to ensure we don't break existing code to this degree.

Currently, CJS compiled via babel and other tools works fine and with full capabilities but differently from some tools. This would cause another wave of needing to rewrite code. In particular I think it would be more realistic to change a default if all the tools agreed on behavior for __esModule but they do not.

@qnighy
Copy link
Author

qnighy commented Nov 20, 2021

I see. I also started to think it unnecessary to change the global default, recalling the original motivation. To provide a better migration path for downstream modules (including application codes), it is enough to give options to these downstream modules. So I'll try to switch behaviors based on the importing module's configuration.

@GeoffreyBooth
Copy link
Member

This is the kind of thing that would be better enabled via a loader rather than a global flag. See https://github.com/nodejs/loaders. Though if it's on the cjs side it might need to be in require.extensions.

@qnighy
Copy link
Author

qnighy commented Feb 18, 2022

Closing it as well: see #40891 (comment) #40902 (comment)

@qnighy qnighy closed this Feb 18, 2022
@qnighy qnighy deleted the cjs-import-interop branch February 18, 2022 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding an option to interpret __esModule as Babel does
6 participants