Return the namespace by default when requiring ESM #507

lukastaegert · 2020-07-24T14:10:22Z

Rollup Plugin Name: `commonjs`

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

List any relevant issue numbers:
#481
Resolves #400
Resolves #491
Resolves #431

Description

This is my attempt to implement Improvement 3, Improvement 4a, Improvement 4b and Improvement 6 from #481. This should be mostly working but requires a little more refinement. Also I refactored the index.js file by extracting some stuff to make it a little more readable again.

First this will change the default behaviour when requiring ES modules from a CommonJS context to return the namespace, i.e. require('foo') by default becomes import * as foo from 'foo'. This goes both for modules in the graph as well as external dependencies. This is in line with what e.g. Webpack and other tools do and what Node will likely do if they would ever support this.

As this is likely to break some code-bases and might also blow up code bases due to unnecessary namespace objects being created, there is a new option requireReturnsDefault that allows to customize this behaviour. There is also another option esmExternals that controls if this also applies to external dependencies. It this option is not used, then external imports are rendered as default imports to be compatible with Node importing CJS from ES modules.

There are four possible values for requireReturnsDefault:

false: This is the default, all ES modules return their namespace when required. For external dependencies, no interop is used as the assumption is that here, require returns the namespace.
"auto": This is complementary to how output.exports: 'auto' currently works in Rollup: If there is a default export and no named exports, requiring a module returns that value. In all other cases, the namespace is returned. For external dependencies, a corresponding interop helper is used.
"preferred": This is close to how the plugin used to work: If there is a default export, requiring a module always returns the default export, no matter named exports exist. Again for external dependencies, a slightly different interop helper is used.
true: This will always try to return the default export on require no matter if it actually exists. This can throw if it does not exist. The advantage is that, like false, this does not add an interop helper for external dependencies, keeping the code lean.

Besides setting a global value, a function can be passed to requireReturnsDefault as well. In that case, each required module will be passed once to that function, which can then return any of the values above to fine-tune the code-base.

TODO:

Documentation
Maybe more tests if I forgot some branches
Verify which issues are solved by this
Check if there are more open points from Improvement 3, Improvement 4a, Improvement 4b and Improvement 6 from [commonjs] Discussion and overview of (nearly) all current interop issues + ideas how to move forward #481

lukastaegert · 2020-07-24T16:51:23Z

Docs are updated now.

shellscape · 2020-07-25T02:06:19Z

LGTM so far

guybedford

I agree with the approach in principle, but I think what is being overlooked here is that the major pattern for CJS externals is CJS modules are externals of CJS.

In Node.js if I have require('express') and want to run that as an ES module I need to use import express from 'express'.

So defaulting to treating all externals as ESM externals is the wrong default IMO.

Instead of a requireReturnsDefault option I would recommend an option that allows specifying which externals are ES module externals explicitly, which then can correspond to namespaces.

For example:

const marked = require('marked');
const esmExternal = require('esm-external');

can then use a config like:

esmExternals: ['esm-external']

to handle the namespace case.

Otherwise, if this namespace handling is the default there will be even more default interop woes I fear, since a CommonJS module is the most common external dependency of a CommonJS module even when defining the external boundary.

lukastaegert · 2020-07-26T04:41:47Z

I agree with the approach in principle, but I think what is being overlooked here is that the major pattern for CJS externals is CJS modules are externals of CJS.

Yes, thank for digging into this so early, I actually agree here. So my thought was of course that since we are generating ESM that alls externals would be ESM but as a matter of fact it is much more likely that most externals are CJS. So I will add this option and the logic will be:

if the option is not used or a dependency is not listed, we treat is as requireReturnsDefault: true was supplied for that option, in effect treating it as a default import. Thus the generated ESM output would be directly usable in Node.
Otherwise we run it through requireReturnsDefault and do what I am doing now

Note though that the effect when using cjs as Rollup format will be that Rollup will add another interop to get the default export. This will work of course for CJS dependencies that do not use __esModule, just something to keep in mind. Using a namespace import would not add that interop. I am currently working on a patch for Rollup that will also allow to configure that level of interop to make it disappear, though.

lukastaegert · 2020-07-26T06:12:29Z

Done, see changed documentation.

lukastaegert · 2020-07-26T07:04:15Z

This is now ready for review. There are some dependencies issues that need sorting, though. I can confirm this now resolves both #400 and #491.

lukastaegert · 2020-07-27T04:36:04Z

@shellscape It feels like security issues with packages are rather nasty to fix with pnpm and I would need some guidance here:

It does not seem I can fix it for one package alone, I need to fix it for all packages
Of course lodash is one of the offenders, but I cannot seem to easily figure out which package depends on the outdated lodash version

I could of course update everything for all packages, but no guarantees that this will not introduce more issues down the line. I will try a little more.

danielgindi · 2020-07-27T04:43:10Z

@shellscape It feels like security issues with packages are rather nasty to fix with pnpm and I would need some guidance here:

It does not seem I can fix it for one package alone, I need to fix it for all packages

Of course lodash is one of the offenders, but I cannot seem to easily figure out which package depends on the outdated lodash version

I could of course update everything for all packages, but no guarantees that this will not introduce more issues down the line. I will try a little more.

I don't see the context... Anyway, let npm fix it with the builtin security feature. Or let github do it with automatic pull requests for package-lock.json. Oops we are not using any of those here 😂

lukastaegert · 2020-07-27T05:09:43Z

Ok, it seems we definitely need to do a major ava version bump as well as that one depends on an offending yargs-parser...

lukastaegert · 2020-07-27T12:23:17Z

I updated dependencies to get it working again. The question is how this should be handled in other plugins? Besides the ava update (which is of course only relevant for development), I think I only did minor version updates that should not change functionality.

lukastaegert · 2020-07-27T12:56:53Z

I was also able to add a fix for #431. The problem here only manifested when a CommonJS file or mixed module required a mixed module because the generated proxy module wrongly assumed the file to have CommonJS exports (mixed modules still only have ESM exports).

So provided there is positive feedback from reviewers (@guybedford ?), I think this could be released together with the existing changes on master as the next major version. The remaining changes listed in #481 should be non-breaking and could be added incrementally as they only add options or improve output quality.

shellscape · 2020-07-27T13:18:35Z

@lukastaegert we've run into this before - if you're going to update multiple plugins at once, you need to commit for each individually. otherwise, we cannot version or generate changelogs for them. our versioning and changelog generation works based on commits right now. so we'll need you to do one of two things:

revert the changes to the other plugins, reapply the fix to all of them, commit for each individually
revert the changes to the other plugins, update only commonjs and recommit

If you want to update all of the plugins, I would greatly appreciate it if that were done another another branch that we can just use a regular merge on. If we have to update them all on one branch, then I'll have to do a manual squash merge from my machine to merge the PR. This is why we recommend only working on one plugin in a PR.

lukastaegert · 2020-07-27T13:39:00Z

This is why we recommend only working on one plugin in a PR.

Sure and I would have loved to, it just was not an option any more due to the security issues spanning all plugins and some core dependencies at once. I will see if I can split the update tonight into a separate branch that contains separate commits for all plugins. Once that one is merged, I will remove the dependency updates from this branch so that it can be merged without issues.

shellscape · 2020-07-27T13:43:54Z

Sounds good. I'll add something to contributing about performing dependency updates en masse in a separate branch, and with guidance on how to go about that.

guybedford

Thanks for being open to the suggestion here, this looks like a very flexible approach to me.

Just the one documentation note to make sure it's easy to understand when requireReturnsDefault will apply.

guybedford · 2020-07-27T14:37:28Z

packages/commonjs/README.md

+Type: `boolean | "auto" | "preferred" | ((id: string) => boolean | "auto" | "preferred")`<br>
+Default: `false`
+
+Controls what is returned when requiring an ES module or external dependency from a CommonJS file. By default, this plugin will render it as a namespace import, i.e.


Perhaps clarify in the description directly that this is when esmExternals is enabled only? If I'm reading this right?

frank-dspeed · 2020-07-28T10:44:12Z

Sorry that i see that so late but require should not be able to load ESM only import() inside CJS can import ESM require is bound to always require CJS

in ESM import and import() can load CJS as also createRequire will always load as CJS

lukastaegert · 2020-07-28T12:10:58Z

Yes, this is how Node handles it at the moment and there will likely be a compatibility mode in the future that will flag this. This is not what this PR is about, though. Requiring ESM from CJS has been supported for a long time, just as well as it is supported in Webpack and Parcel. It's just that our implementation is incompatible and partly broken, which is fixed here.
Also to my understanding it is not set in stone that Node might not at some point support this as well: It is mostly technical hurdles preventing this. If this should ever happen, this PR will likely make our implementation compatible with that as well.

frank-dspeed · 2020-07-28T16:21:37Z

@lukastaegert ok as it is only a plugin and easy fork able you got my LGTM+1 but i will go a other direction and will improve stuff like lebab which transpils to es6+ so there will be only es6 + i do not aim CJS compat or UMD compat or iife Compat i think SystemJS for Older Browsers and ESM only is the way to go forward everything else is the past.

BREAKING CHANGES: By default, require expressions will return the namespace of internal and external modules

lukastaegert · 2020-07-31T15:12:07Z

@shellscape I have removed the dependency updates from this branch and rebased to master. One thing I noticed is that I DID forget a parameter to leave README docs unchanged, which was --trailing-comma none. I hope it is ok I added it here in a single commit for all as it should have no impact on anything but keep the formatting unchanged in case any readme is touched. So if CI runs through, this would be ready for merge and actually release from my side.

lukastaegert · 2020-07-31T15:31:30Z

packages/auto-install/test/npm.js

@@ -17,7 +17,7 @@ const pkgFile = join(cwd, 'package.json');
 process.chdir(cwd);

 test('invalid manager', (t) => {
-  t.timeout(30000);
+  t.timeout(50000);


I kept seeing random timeouts on these tests, this seems to help a little. Sometimes on Windows, npm seems to be really slow..

shellscape · 2020-08-01T12:58:34Z

Whew that was a biggie! Thanks for working on this and for everyone's patience working through it. I'll get this released today.

…ollup#507) * feat(commonjs): require ESM namespace by default, allow configuration BREAKING CHANGES: By default, require expressions will return the namespace of internal and external modules * chore(commonjs): refactor and clean up code * chore(commonjs): add documentation * feat(commonjs): add esmExternals option * fix(commonjs): handle requiring mixed ES modules * chore(commonjs): adapt to updated dependencies

lukastaegert mentioned this pull request Jul 24, 2020

[commonjs] Discussion and overview of (nearly) all current interop issues + ideas how to move forward #481

Closed

lukastaegert force-pushed the require-esm-returns-namespace branch from 2d9577a to 325eba7 Compare July 24, 2020 16:50

shellscape requested review from danielgindi, NotWoods, Andarist, LarsDenBakker and guybedford July 25, 2020 02:05

guybedford requested changes Jul 25, 2020

View reviewed changes

lukastaegert force-pushed the require-esm-returns-namespace branch 2 times, most recently from e4dc686 to 76411c0 Compare July 26, 2020 06:12

lukastaegert force-pushed the require-esm-returns-namespace branch 2 times, most recently from b488fa4 to bd566e7 Compare July 26, 2020 06:50

lukastaegert marked this pull request as ready for review July 26, 2020 07:03

lukastaegert force-pushed the require-esm-returns-namespace branch from 6d952c6 to bf5002d Compare July 27, 2020 11:03

lukastaegert added the 🔌 plugin-commonjs label Jul 27, 2020

lukastaegert force-pushed the require-esm-returns-namespace branch 5 times, most recently from df34fff to 2cd399a Compare July 27, 2020 12:21

lukastaegert force-pushed the require-esm-returns-namespace branch from 2cd399a to 3b10706 Compare July 27, 2020 12:30

lukastaegert force-pushed the require-esm-returns-namespace branch from 3b10706 to d509183 Compare July 27, 2020 12:46

lukastaegert force-pushed the require-esm-returns-namespace branch from d509183 to b5302c1 Compare July 27, 2020 13:00

guybedford approved these changes Jul 27, 2020

View reviewed changes

shellscape mentioned this pull request Jul 28, 2020

Fix: nodejs esm interop #512

Closed

12 tasks

lukastaegert added 5 commits July 31, 2020 16:45

feat(commonjs): require ESM namespace by default, allow configuration

24cf54c

BREAKING CHANGES: By default, require expressions will return the namespace of internal and external modules

chore(commonjs): refactor and clean up code

5581bc2

chore(commonjs): add documentation

436c84f

feat(commonjs): add esmExternals option

ed024de

fix(commonjs): handle requiring mixed ES modules

3364284

lukastaegert force-pushed the require-esm-returns-namespace branch 3 times, most recently from 0c2313e to b1f5a2c Compare July 31, 2020 15:11

chore(commonjs): adapt to updated dependencies

c2fb584

lukastaegert force-pushed the require-esm-returns-namespace branch from b1f5a2c to c2fb584 Compare July 31, 2020 15:19

lukastaegert commented Jul 31, 2020

View reviewed changes

shellscape merged commit 621768b into master Aug 1, 2020

shellscape deleted the require-esm-returns-namespace branch August 1, 2020 12:58

nicolo-ribaudo mentioned this pull request Feb 10, 2021

Use conditional exports in @babel/runtime for CJS/ESM babel/babel#12632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return the namespace by default when requiring ESM #507

Return the namespace by default when requiring ESM #507

lukastaegert commented Jul 24, 2020 •

edited

lukastaegert commented Jul 24, 2020

shellscape commented Jul 25, 2020

guybedford left a comment

lukastaegert commented Jul 26, 2020

lukastaegert commented Jul 26, 2020

lukastaegert commented Jul 26, 2020

lukastaegert commented Jul 27, 2020

danielgindi commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

shellscape commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

shellscape commented Jul 27, 2020

guybedford left a comment

guybedford Jul 27, 2020

frank-dspeed commented Jul 28, 2020 •

edited

lukastaegert commented Jul 28, 2020

frank-dspeed commented Jul 28, 2020

lukastaegert commented Jul 31, 2020

lukastaegert Jul 31, 2020

shellscape commented Aug 1, 2020

Return the namespace by default when requiring ESM #507

Return the namespace by default when requiring ESM #507

Conversation

lukastaegert commented Jul 24, 2020 • edited

Rollup Plugin Name: commonjs

Description

lukastaegert commented Jul 24, 2020

shellscape commented Jul 25, 2020

guybedford left a comment

Choose a reason for hiding this comment

lukastaegert commented Jul 26, 2020

lukastaegert commented Jul 26, 2020

lukastaegert commented Jul 26, 2020

lukastaegert commented Jul 27, 2020

danielgindi commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

shellscape commented Jul 27, 2020

lukastaegert commented Jul 27, 2020

shellscape commented Jul 27, 2020

guybedford left a comment

Choose a reason for hiding this comment

guybedford Jul 27, 2020

Choose a reason for hiding this comment

frank-dspeed commented Jul 28, 2020 • edited

lukastaegert commented Jul 28, 2020

frank-dspeed commented Jul 28, 2020

lukastaegert commented Jul 31, 2020

lukastaegert Jul 31, 2020

Choose a reason for hiding this comment

shellscape commented Aug 1, 2020

lukastaegert commented Jul 24, 2020 •

edited

Rollup Plugin Name: `commonjs`

frank-dspeed commented Jul 28, 2020 •

edited