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
Remove useESModules
in favor of conditional exports for @babel/runtime
#12295
Remove useESModules
in favor of conditional exports for @babel/runtime
#12295
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/31682/ |
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 948c463:
|
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.
Just a single comment so far but I will also have to check out this branch to see what exactly is being built, the dir structure etc - will be easier to check some things that way. Gonna do that later, hopefully today.
for (const helperName of helpers.list) { | ||
const helperPath = path.join("helpers", helperName); | ||
helperSubExports[`./${helperPath}`] = { | ||
import: writeHelperFile(runtimeName, pkgDirname, helperPath, helperName, { |
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.
for the ideal experience, the very same value should be repeated for a priority "module"
condition that is supported by webpack (and will most likely be supported by Rollup as well), in node require
cant load ESM so it can lead to the same module being loaded twice (once for ESM loader and once for the CJS loader) which ain't ideal for the web consumers so this condition allows the same file (authored in ESM) to be loaded for both loaders, so this behaves pretty much as the package.json#module
Ok, I've managed to check this quickly while hanging over the phone - one additional thing. I believe that it would generally be best (and required for As those modules are mostly consumed automatically through plugins this shouldn't be much of a problem even for CJS-only consumers. |
cf95c47
to
d66b663
Compare
64b862b
to
426ef52
Compare
return arrayWithHoles(arr) || iterableToArray(arr) || unsupportedIterableToArray(arr) || nonIterableRest(); | ||
} | ||
|
||
module.exports = _toArray; |
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.
as mentioned on Slack - this won't work because one can write:
// node_modules/cast-array/index.js
const toArray = require("@babel/runtime/helpers/toArray")
module.exports = maybeArr => toArray(maybeArr) // it doesn't quite matter what's here, the prior line is important
and then one can consume such a package using webpack (4 and 5 included, maybe up to 2) with:
import castArray from 'cast-array'
castArray(document.querySelectorAll('.foo'))
Here webpack would consume the first module (cast-array
) and assume that what it has imported has the shape of module.exports = { default: toArray }
which would fail loudly at runtime (or at build time? not sure actually).
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.
one can write
This package is only meant to be consumed by generated code, not manually. It webpack throws in that case, or generates invalid code, that's ok. Babel will always inject imports like require("@babel/runtime/helpers/toArray").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.
Whats the reason for the direct assignment then? Why it exists? When it will be used?
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.
So that we can emit this code in Node.js:
import toArray from "@babel/runtime/helpers/toArray"
instead of
import toArray_moduleExports from "@babel/runtime/helpers/toArray"
const toArray = toArray_moduleExports.default;
because Node.js's default import is the value of module.exports
.
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.
Could u describe the scenario end to end?
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.
I believe that a problem is that the output consumer might not be a final consumer. The transform-runtime plugin is often used when building packages and im mostly worried about scenarios related to that while i think u are describing the former scenario - correct me if i have misunderstood your intention
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.
There are different usages I'm thinking of, and I don't think that whether the file is a pre-compiled package or a compiled source file of the final app matters.
.mjs
output, used in Node.js (:warning: This is where we needmodule.exports
to be the helper itself)This will loadimport toArray from "@babel/runtime/helpers/toArray";
@babel/runtime/helpers/toArray/index.cjs
, and it'smodule.exports
will be used by Node.js as the value oftoArray
. Internally, Node.js does something likeconst toArray = require("@babel/runtime/helpers/toArray/index.cjs");
.cjs
output, used in Node.jsThis will loadconst toArray = require("@babel/runtime/helpers/toArray").default;
@babel/runtime/helpers/toArray/index.cjs
, and it'smodule.exports.default
will be used as the value oftoArray
..mjs
output, used in [bundler]This will loadimport toArray from "@babel/runtime/helpers/toArray";
@babel/runtime/helpers/toArray/index.mjs
, and Webpack will internally use thedefault
property of that file's namespace object (import("@babel/runtime/helpers/toArray/index.mjs").default
)..cjs
output, used in [bundler]This will loadconst toArray = require("@babel/runtime/helpers/toArray").default;
@babel/runtime/helpers/toArray/index.mjs
, and it'sdefault
will be used as the value oftoArray
(import("@babel/runtime/helpers/toArray/index.mjs").default
)..mjs
output, directly used in browsers (for example, with Snowpack)This will loadimport toArray from "@babel/runtime/helpers/toArray";
@babel/runtime/helpers/toArray/index.mjs
, and use it'sdefault
export as the value oftoArray
(similarly to bundlers).cjs
output, directly used in browsers (for example, with Snowpack)
It doesn't work (as expected).
By doing so, Node.js will only load the .cjs
file and [bundler] will only load the .mjs
file, and they will never be loaded together.
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.
I think this indeed might work 👍 Would not bet money on it though 😉 but that's mainly because how the overall story is complicated, not because of the chosen approach
import _inherits from "@babel/runtime-corejs3/helpers/esm/inherits"; | ||
import _possibleConstructorReturn from "@babel/runtime-corejs3/helpers/esm/possibleConstructorReturn"; | ||
import _getPrototypeOf from "@babel/runtime-corejs3/helpers/esm/getPrototypeOf"; | ||
import _classCallCheck from "@babel/runtime-corejs3/helpers/classCallCheck"; |
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.
is useES6Modules
a thing (it's in the fixture path)? Seems like maybe this test is not relevant any longer?
@@ -1,10 +1,10 @@ | |||
var _classCallCheck = require("@babel/runtime/helpers/classCallCheck"); | |||
var _classCallCheck = require("@babel/runtime/helpers/classCallCheck").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.
similarly here but for other reasons - this PR removes useESModules
so this fixture can be removed
@@ -1,10 +1,10 @@ | |||
var _classCallCheck = require("@babel/runtime/helpers/classCallCheck"); | |||
var _classCallCheck = require("@babel/runtime/helpers/classCallCheck").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.
import _inherits from "@babel/runtime/helpers/esm/inherits"; | ||
import _possibleConstructorReturn from "@babel/runtime/helpers/esm/possibleConstructorReturn"; | ||
import _getPrototypeOf from "@babel/runtime/helpers/esm/getPrototypeOf"; | ||
import _classCallCheck from "@babel/runtime/helpers/classCallCheck"; |
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.
import _inherits from "@babel/runtime/helpers/esm/inherits"; | ||
import _possibleConstructorReturn from "@babel/runtime/helpers/esm/possibleConstructorReturn"; | ||
import _getPrototypeOf from "@babel/runtime/helpers/esm/getPrototypeOf"; | ||
import _classCallCheck from "@babel/runtime/helpers/classCallCheck"; |
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.
} | ||
|
||
module.exports = _toArray; | ||
module.exports["default"] = module.exports; |
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.
Are we sure that the sole existence of .default
is enough here? It seems that it might be because of the same thing being assigned both to module.exports
and module.exports.default
.
When consuming ESM file webpack 4 would load this file directly (as it would just fallback to /index.js
because it's not aware of pkg.json#exports
) and if it's using the exact same logic as Babel:
function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }
this would work even though __esModule
is not present here 🤔 I'm wondering though if it still wouldn't be worth adding __esModule
as maybe some other tools treat this different somehow? Although not sure how that would look like (such a tool's interop logic). Just thinking out loud.
Moved to #12632 |
Using Node.js'
exports
mappings, we can get rid of the"useESModules": true
option: it will automatically pick up the preferred version based on how the helper is requried/imported.exports
are also supported by webpack and rollup, but I have structured the files in a way that, even ifexports
isn't supported by some tool, it will load the CJS version because the file is namedindex.js
.In order to avoid loading both the CommonJS and the ESM version, which can lead to duplicated state, I structured
exports
so that:require
it "just works", when usingimport
it will get the value ofmodule.exports
. (Thenode
export condition).mjs
version. Webpack requires that the CJS and ESM files have the same interface, so the CJS helpers also export their function asmodule.exports.default
. (Themodule
condition)module
/node
conditions, it will always load the.mjs
version. (Thedefault
condition)