module: CJS exports detection for modules with __esModule export #33416
Conversation
nodejs-github-bot
added
the
lib / src
There was a problem hiding this comment.
I'm against this feature in general, and the specific implementation needs to support running without access to eval (running with --disallow-code-generation-from-strings). The exact behaviour of this will also need to be exhaustively documented and matched to node's semver cycle.
There was a problem hiding this comment.
I like the general direction of this. It offers a pretty reasonable solution to the incremental adoption problem (see comment on @bmeck's repo). It would be great if we could offer an answer and this is the closest thing I'm aware of.
(I didn't mention it here but I agree with the implementation concerns around eval raised by @devsnek, also the requirement for a way to lock down the semantics to match versioning expectations.)
guybedford
force-pushed
the
cjs-export-detection
branch
from
0227d6e
to
0bb148f
Compare
|
@devsnek I've removed the Does that cover your concerns? |
guybedford
force-pushed
the
cjs-export-detection
branch
from
0bb148f
to
c97caeb
Compare
|
@guybedford What exactly is the effect of I think it would address @devsnek’s main concern (that this makes otherwise non-breaking changes to CJS code breaking, which I agree is bad) if this only applied to modules that export |
|
@addaleax the only effect of This feature was specifically requested by @jkrems to support the standard Babel output that transpiles: export default value;to: Object.defineProperty(exports, '__esModule', { value: true });
exports.default = value;The justification is that by supporting |
|
@guybedford I think the technical concerns I mentioned above were addressed. That being said, I am still -1 on this pr. I've exhaustively outlined the issues I have with cjs export detection in nodejs/modules#509, and in the case of this pr the issues I have with not using V8's parser. |
|
@devsnek, as far as I've been able to condense your feedback from that thread it seems to come down to the following argument:
You therefore argue that users who want to support ESM consumers with named exports should write ESM wrappers instead. Let me know if that seems like an adequate summary of your opinion here? This still leaves older packages with authors who maybe don't want to publish an ESM wrapper, or simply are no longer maintaining the package. Are you ok with not supporting named exports for these use cases? Remind me as well - were you for or against early CJS execution? Between that and this those are the only ways at this point of getting users named exports based on their existing expectations with modules tools. |
|
@guybedford i'm not going to say i'm content with lack of named exports for old packages, but i think having consistent behaviour is much better for dev experience than named exports. |
guybedford
force-pushed
the
cjs-export-detection
branch
from
c27b4cf
to
d9fe4f4
Compare
guybedford
added
the
modules-agenda
|
I wrote some code to test this out: https://github.com/GeoffreyBooth/node-test-commonjs-named-exports. It took a lot more code and effort than expected, so please check my work in case my results are tainted by bugs. There's a README in the repo. Anyway the bottom line is that of 936 packages tested (from within npm's top 1000):
And of all the named exports of all of those packages, 3,390 of 22,579 CommonJS named exports (15%) were detected successfully. Packages with all CommonJS named exports detected:
Packages with some but not all CommonJS named exports detected:
Packages with no CommonJS named exports detected:
There aren't 1000 packages tested because the rest had various issues like being Windows-only, or failing to build on my machine, etc. But I think this sample should be representative. |
|
@GeoffreyBooth it would be interesting to also get results that ignore |
You're welcome to check out the repo and modify it however you wish. Please let me know what you find. I already excluded exports named |
|
A concern I have with the handling of import pkg1 from 'pkg1';
pkg1.default('call the default export');
pkg1.named1('call the named1 export');The above is the current way to import CJS that babel produced by transpiling ESM. |
|
But conversely, there's no package author who wanted to author a default export, have it be transpiled, and then reimported with a default import that you then, further, needed to say |
|
@coreyfarrell that doesn't seem right; if the CJS was produced from babel, importing its default from babel will be |
|
@wesleytodd that's fair. I know this will break a couple imports I'm using in private code but it will make that code less ugly. @ljharb I was referring to using node.js ES module loader to import a package produced by babel (not using babel in my own package). |
|
Ah, the you’re right it would be a breaking change, but modules are experimental, and imo if a CJS module has a “.default” property i consider that a bug :-) since its exposing Babel as an impl detail. |
When I read this bullet, I thought that meant "the automagical parser detector failed in 33% of the cases." But it turns out the situation is much better than this! I noticed that
Today in Node 14.3, So I started manually poking around the top 10 packages with "no named exports detected": I suggest that this test be rewritten to distinguish four cases:
|
It's about user expectations. For example, this works today: const { shuffle } = require('lodash');
// shuffle is a usable functionSo therefore, users would expect to be able to refactor into this: import { shuffle } from 'lodash';
// throwsBecause this |
|
@jdalton is it your intention that lodash have named exports? Author intention is what I think matters here, not user expectations. |
|
lodash is meant to have named exports, and I would imagine many of the others are as well. |
|
@devsnek i don't see anything on its readme that implies that whatsoever, and its use of |
|
oh you already pinged him, I missed that. guess the app didn't update the comments properly :( |
|
Closing as #35249 has now landed here. Thanks so much everyone for the feedback on this feature. |
MylesBorins
removed
modules-agenda
This PR provides named exports support for transpiled CommonJS modules (restriction to __esModule sources added based on feedback from discussion at #33416 (comment)) to work out correctly using a very fast Wasm-based analysis process that runs before CJS execution, allowing
import { name } from 'transpiled-esm-to-cjs'as most JS users would expect, without incurring any significant performance penalty.This analysis is cached in a weakmap associated with the module object, which also stores the early-accessed source cache for usage on the actual load.
The approach is very similar to the approach TypeScript uses for detection of CommonJS named exports during the binding phase as we discussed in the last modules meeting with @weswigham.
The major concern for a parsing approach is the performance overhead of running a full parse of all CommonJS modules that are imported from an ES module. Acorn parse time is usually in the 10s of miliseconds for standard size source files to 100s of miliseconds for large sources. Scaling this up to multiple dependencies could introduce some loading overhead.
This PR incorporates a fork of es-module-lexer, cjs-module-lexer (https://github.com/guybedford/cjs-module-lexer) to handle this exports analysis with Web Assembly so that the list of named exports for CJS can be known at link time via a very fast source Wasm lexer process (sub millisecond for most sources).
es-module-lexer is used in many current ES module tooling projects, and has been battle tested against a wide range of real-world JS sources over the past two years. It supports the full JS grammar, including comments, strings, template strings and regex / division operator ambiguity.
cjs-module-lexer forks to handle CommonJS detecting named exports extractions based on the rules outlined at https://github.com/guybedford/cjs-module-lexer#supported. It is lexing matcher for patterns of
exports.etc, including a Webpack heuristic as well.Limitations include that some minified sources that obscure the
exportsbinding cannot be parsed as the analysis does not do value tracking at all as it only detects common token patterns forexportsetc.In testing the approach has shown to work well for standard TypeScript and Babel transpilations. Identifiers are always filtered to be valid JS identifiers.
The major edge case this differs from current semantics with bundlers on is that the default export is retained as the full module.exports instance for backwards compat and consistency. This is important for Node.js semantic consistency I think.
//cc @nodejs/modules-active-members
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes