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
Implement importInterop: "node"
option for module transforms
#12838
Conversation
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:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45028/ |
Excellent to see this, and thanks for making a path here. /cc @lukastaegert may be interested in this as well. |
6138a24
to
42105c0
Compare
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") { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| "default" | ||
| "namespace" | ||
| "node-default" | ||
| "node-namespace" | ||
| "none"; |
There was a problem hiding this comment.
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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is metadata.interop
?
There was a problem hiding this comment.
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".
556c0a9
to
d195510
Compare
This PR introduces a new option (
importInterop: "babel" | "node" | "none"
) to the CJS, AMD and UMD plugins.importInterop: "babel"
is the current default behaviorimportInterop: "node"
will compileimport
statements in the same way as they are handled by Node.jsimportInterop: "none"
is the same as the currentnoInterop: true
. It compiles imports as if it wasimportInterop: "babel"
, but always usingmodule.exports.default
as the default import without checking if__esModule
is true. In the next majornoInterop
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 thelazy
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):
And this ECMAScript module in the user's project:
When running in Node.js, it logs
When running after compiling the ESM file with Babel, it logs
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: