Review requested:

• @nodejs/modules

what do the numbers look like on this one?

As someone less familiar with the code here, but interested in improved ecosystem interop, I really like this approach and think it would greatly help a lot of the ecosystem.

@guybedford looks like this needs a rebase

Those numbers aren't looking too good.

I made several updates to my test suite. I made the test run in a child process, thereby working around the bug that was previously causing crashes. I also installed the packages to test in separate subfolders for each set of 200 packages, to work around how npm install get exponentially slower as a package.json gets into thousands of dependencies. I also improved the test itself, avoiding mistakenly detecting non-enumerable properties as intended named exports; you can see the full test here. I also rewrote the presentation of the results to make them easier to follow based on people’s preferences.

The upshot of all of this is that now I can install packages so much faster that I changed “top 3,000” to “top 20,000,” though there are still many packages that get excluded from testing for various reasons (expecting browser or Windows environments, etc.). The other thing I noticed is that the dataset I’m using for “top npm packages” hasn’t been updated since 2016, even though its readme says it gets updated nightly. I think this isn’t as bad as it sounds: it’s just a list of package names, not names with versions, and I’ve been installing the latest versions of all these packages. The point of this PR is to improve compatibility with older npm packages that are still popular, so a dataset of packages that were popular a few years ago (but the latest code for those packages) is arguaby more appropriate than today’s most popular.

Of all 16,107 packages installed and tested:

• 68% (10,876 of 16,107) packages had either only a default export in CommonJS and therefore also in ESM (6642 packages) or all named exports detected in CommonJS were also detected in ESM (4234 packages).
• 32% (5,231 of 16,107) packages had either some but not all CommonJS exports detected in ESM (1128 packages) or no CommonJS exports detected in ESM (4103 packages). Of the packages that had some but not all CommonJS exports detected:
  • 4% (46 of 1,128) packages had at least 80% of CommonJS exports detected.
  • 10% (118 of 1,128) packages had at least 60% of CommonJS exports detected.
  • 29% (329 of 1,128) packages had at least 40% of CommonJS exports detected.
  • 53% (603 of 1,128) packages had at least 20% of CommonJS exports detected.

Of 364 packages that had a readme encouraging the use of named exports:

• 75% (274 of 364) packages had either only a default export in CommonJS and therefore also in ESM (5 packages) or all named exports detected in CommonJS were also detected in ESM (269 packages).
• 25% (90 of 364) packages had either some but not all CommonJS exports detected in ESM (26 packages) or no CommonJS exports detected in ESM (64 packages). Of the packages that had some but not all CommonJS exports detected:
  • 4% (1 of 26) packages had at least 80% of CommonJS exports detected.
  • 12% (3 of 26) packages had at least 60% of CommonJS exports detected.
  • 35% (9 of 26) packages had at least 40% of CommonJS exports detected.
  • 50% (13 of 26) packages had at least 20% of CommonJS exports detected.

Of 700 packages generated via transpilation:

• 92% (644 of 700) packages had either only a default export in CommonJS and therefore also in ESM (99 packages) or all named exports detected in CommonJS were also detected in ESM (545 packages).
• 8% (56 of 700) packages had either some but not all CommonJS exports detected in ESM (16 packages) or no CommonJS exports detected in ESM (40 packages). Of the packages that had some but not all CommonJS exports detected:
  • 19% (3 of 16) packages had at least 80% of CommonJS exports detected.
  • 44% (7 of 16) packages had at least 60% of CommonJS exports detected.
  • 50% (8 of 16) packages had at least 40% of CommonJS exports detected.
  • 56% (9 of 16) packages had at least 20% of CommonJS exports detected.

I also found a data source for the top 1,000 packages as of August 2019. About half of these packages are the same as the 2016 top 1,000. Here are the results for this list:

Of all 921 packages installed and tested:

• 64% (594 of 921) packages had either only a default export in CommonJS and therefore also in ESM (306 packages) or all named exports detected in CommonJS were also detected in ESM (288 packages).
• 36% (327 of 921) packages had either some but not all CommonJS exports detected in ESM (71 packages) or no CommonJS exports detected in ESM (256 packages). Of the packages that had some but not all CommonJS exports detected:
  • 8% (6 of 71) packages had at least 80% of CommonJS exports detected.
  • 15% (11 of 71) packages had at least 60% of CommonJS exports detected.
  • 28% (20 of 71) packages had at least 40% of CommonJS exports detected.
  • 44% (31 of 71) packages had at least 20% of CommonJS exports detected.

Of 40 packages that had a readme encouraging the use of named exports:

• 60% (24 of 40) packages had either only a default export in CommonJS and therefore also in ESM (0 packages) or all named exports detected in CommonJS were also detected in ESM (24 packages).
• 40% (16 of 40) packages had either some but not all CommonJS exports detected in ESM (7 packages) or no CommonJS exports detected in ESM (9 packages). Of the packages that had some but not all CommonJS exports detected:
  • 0% (0 of 7) packages had at least 80% of CommonJS exports detected.
  • 14% (1 of 7) packages had at least 60% of CommonJS exports detected.
  • 43% (3 of 7) packages had at least 40% of CommonJS exports detected.
  • 43% (3 of 7) packages had at least 20% of CommonJS exports detected.

Of 43 packages generated via transpilation:

• 88% (38 of 43) packages had either only a default export in CommonJS and therefore also in ESM (1 packages) or all named exports detected in CommonJS were also detected in ESM (37 packages).
• 12% (5 of 43) packages had either some but not all CommonJS exports detected in ESM (4 packages) or no CommonJS exports detected in ESM (1 packages). Of the packages that had some but not all CommonJS exports detected:
  • 0% (0 of 4) packages had at least 80% of CommonJS exports detected.
  • 25% (1 of 4) packages had at least 60% of CommonJS exports detected.
  • 50% (2 of 4) packages had at least 40% of CommonJS exports detected.
  • 50% (2 of 4) packages had at least 20% of CommonJS exports detected.

(approval on the approach, haven't reviewed the code yet)

@nodejs/tsc @nodejs/collaborators @nodejs/modules

Assuming that @guybedford get the updated implementation in time I plan to land this on Monday evening (after sun down because it's Yom Kippur) so I can kick off a 14.x release that includes this on Tuesday. If anyone has an objection please voice it ASAP.

Feedback added. Finally finished updating this PR to the latest cjs-module-lexer JS conversion at https://github.com/guybedford/cjs-module-lexer.

It doesn't have to be done in this PR, but I think it's important that we have a section in the Node.js documentation that explains to module authors which CommonJS export patterns are supported and which are not. Linking to https://github.com/guybedford/cjs-module-lexer is not enough because the "head" version of the module will evolve faster than what is actually released in Node.

@targos cjs-module-lexer was created specifically for this PR and would be maintained based on Node.js needs. Perhaps one option would be to move the entire repo into the Node.js organization? Let me know if that sounds like an approach that might work here and if so what the next steps might be.

Alternatively I'd be happy to move the grammar into something here, I'm just weary of putting it into the esm.md file as there's already a lot of info. I actually recently wanted to create a PR to even "unhide" the resolver spec already because a lot of people aren't seeing that when it's important info too. We are at risk of losing info in the thick of it all if we don't actively whittle these things down from time to time.

@guybedford What I'm concerned about is that the contents of https://github.com/guybedford/cjs-module-lexer can change at any time. Even if Node.js quickly follows the changes, previous versions of the documentation would link to the latest version of the lexer, and that would not correspond to the behavior in the released Node.js version. One solution could be to link to the README at a specific commit SHA (the one that corresponds to the code in this PR), but I'd still prefer we have a minimal description in the Node.js docs about the supported patterns (Linking to a commit SHA has drawbacks. For example we could not enhance the documentation). It doesn't have to describe the grammar precisely, but could briefly explain with some code examples the patterns that are definitely supported and the ones that aren't.

@targos these are great points. I spent some time today working to heavily clarify the readme of cjs-module-lexer to properly inform the exact semantics, and making sure this was in sync with the code. I've released a 0.3.1 and set the docs link to this exact version readme.

I'm not sure it is possible to summarize the points more simply than what is there. I do think treating that as the primary source here may be best.

I've posted nodejs/admin#557 to follow-up on transferring the repo to the Node.js org. It doesn't have to happen immediately but I do feel it might be the better structure here as I do think of this as Node.js core code effectively now.

I've then also gone through the docs here and reworked it quite a bit too to very clearly focus on the namespace interop and how it behaves exactly. I think it's important to make this the focus over the exact heuristical detection process, which can for most users be considered a sort of "black box".

Specifically on that point, users writing CommonJS shouldn't be "designing" their CommonJS to support this detection. In those cases they should rather just ship an ES module wrapper. What we are more concerned about is how users import CommonJS from ESM and their understanding of this process, and there specifically for CommonJS projects that aren't necessarily well-maintained or being updated with new ESM support.

I know it's not exactly what you've requested, but I hope these clarifications make sense? The new exports detection section is at https://github.com/guybedford/cjs-module-lexer#parsing-examples, and just feels a bit too long to inline into this page to me.