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

Implement importInterop: "node" option for module transforms #12838

Merged
merged 4 commits into from Apr 28, 2021

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 20, 2021

Q                       A
Fixed Issues? Fixes #7294, fixes #7998
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link Docs: babel/website#2476
Any Dependency Changes?
License MIT

This PR introduces a new option (importInterop: "babel" | "node" | "none") to the CJS, AMD and UMD plugins.

  • importInterop: "babel" is the current default behavior
  • importInterop: "node" will compile import statements in the same way as they are handled by Node.js
  • importInterop: "none" is the same as the current noInterop: true. It compiles imports as if it was importInterop: "babel", but always using module.exports.default as the default import without checking if __esModule is true. In the next major noInterop should be removed in favor of this option.

importInterop can also be a function, that receives the import source as a parameter and returns "babel" | "node" | "none". This is especially useful since you usually want different behaviors depending on the imported module (for example, internal vs external). It's similar to how the lazy option accepts a function.

cc @guybedford (since you might be interested in this)


Motivation

Currently it's impossible to write a mode whose import statements work both natively in Node.js and when compiled with Babel (assuming that the imported module is a CJS script, otherwise it cannot be require()d when transpiling).

Consider this CJS scropt (it's particularly problematic when this is a dependency, not just a separate file in the project):

module.exports = exports = function moduleExports() {};
exports.__esModule = true;
exports.default = "default";
exports.named = "named";

And this ECMAScript module in the user's project:

import def, { named } from "cjs-dependency";
import * as ns from "cjs-dependency";

console.log(def);
console.log(named);
console.log(ns);

When running in Node.js, it logs

> function moduleExports() {}
> "named"
> { default: function moduleExports() {}, named: "named" }

When running after compiling the ESM file with Babel, it logs

> "default";
> "named";
> function moduleExports() {}

This is a big problem when trying to gradually migrate from CJS to ESM: it's not possible to generate and ESM and a CJS working outputs from a single source.

The community will start to migrate to ESM (especially dual packages) soon, now that it's becoming mainstream, and many other projects will be affected by this problem.


Since I'm trying to convert our source code to be compatible with the Node.js behavior (to prepare it for native ESM, but still transpiling it to CJS for now), I had to introduce a custom plugin in #12795 to transform import statements in a Node.js-compatible way.

After this PR, #12795 could be reverted and replaced with this:

["@babel/transform-modules-commonjs", {
  importInterop(source) {
    if (source.startsWith(".") || source.startsWith("@babel/")) return "none";
    return "node";
  }
}]

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories area: modules labels Feb 20, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 20, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d195510:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 20, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45028/

@guybedford
Copy link
Contributor

Excellent to see this, and thanks for making a path here. /cc @lukastaegert may be interested in this as well.

if (cache && cache.has(obj)) {
return cache.get(obj);
}

var newObj = {};
var hasPropertyDescriptor = Object.defineProperty && Object.getOwnPropertyDescriptor;
for (var key in obj) {
if (Object.prototype.hasOwnProperty.call(obj, key)) {
if (Object.prototype.hasOwnProperty.call(obj, key) && key !== "default") {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Switch the order

return obj;
}

if (obj === null || (typeof obj !== "object" && typeof obj !== "function")) {
return { default: obj }
}

var cache = _getRequireWildcardCache();
var cache = _getRequireWildcardCache(+!!nodeInterop);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Are we trying to save bytes? It'd be easier to grok with a key name, instead of boolean->int conversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but given that we usually don't worry too much about that I can make it more readable.

Comment on lines 25 to 29
| "default"
| "namespace"
| "node-default"
| "node-namespace"
| "none";
Copy link
Member

Choose a reason for hiding this comment

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

Can we add comments explaining what the difference is between these modes?


if (resolvedInterop === "none") {
metadata.interop = "none";
} else if (resolvedInterop === "node" && metadata.interop === "namespace") {
Copy link
Member

Choose a reason for hiding this comment

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

What is metadata.interop?

Copy link
Member Author

Choose a reason for hiding this comment

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

It tells what interop function we need for the import:

  • default -> default
  • named -> none
  • default+named -> namespace
  • namespace -> namespace

Then in this function we specialize between "babel interop" and "node interop".

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Apr 7, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit be03be1 into babel:main Apr 28, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the modules-node-interop branch April 28, 2021 16:22
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: modules outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
5 participants