fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501

lukastaegert · 2020-07-17T19:35:18Z

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:
Part of #481
Resolves #411
Resolves #454

Description

The main part of this PR is to re-introduce proper interop handling when importing transpiled ESM files, i.e. CJS files with exports.__esModule: true. It now relies on a new Rollup feature to take synthetic named exports from an arbitrary export, in this case __moduleExports. This export will not become part of the public interface of entry points and not show up in reified namespace objects.

In contrast to what I wrote in #481, I again used an interop function because due to the PURE annotation, it allowed for better tree-shaking. The interop I used is actually identical to the old one, which means if there is no default property, the default export falls back to the namespace. This is not according to specs but is needed to prevent consumers that use Rollup's old interop pattern from failing, see the comment I added in the code and rollup/rollup-plugin-commonjs#224
A breaking change I added is that now CJS files will again always expose a default export, even if they are entry points. The reason is that the old logic was very error prone and often did not detect reliably if later dependencies would import something from an entry point. Once we add the exposedExports option I recommended in #481, this should be easily fixable manually by the user. For now, they can just use an ESM wrapper as their entry point.

Also, I changed how the helpers are imported when using dynamic requires. More precisely, I added an ad-hoc wrapper that just reexports the commonjsRegister helper as default export and required that from CJS which prevented the creation of an unneeded namespace object and allowed unused helpers to be tree-shaken away.

Last, I updated all dependencies as I needed to update the Rollup dependency anyway. Another breaking change is the new peer dependency version of Rollup.

Due to the pressing nature of this fix I would recommend to release rather sooner than later, even though the other fixes in #481 will likely need yet another major version.

lukastaegert · 2020-07-18T06:13:42Z

This is now ready for review and could potentially be released as a new major. The rollup peer dependency has been updated to ^2.22.0.

lukastaegert · 2020-07-18T06:17:14Z

Apparently some local paths were re-introduced into the bundle, I will need to check that. Back to draft it is.

lukastaegert · 2020-07-18T12:29:26Z

Ok, all fixed now, please have a look. I very much recommend studying the diff of the function test snapshots at https://github.com/rollup/plugins/pull/501/files#diff-b197c1539f3dea1e33fceb0a1ace63f3 as they can explain the nature of the changes better, and also take a look at the added tests.

BREAKING CHANGES: updates rollup peerDependency to ^2.22.0

BREAKING CHANGES: CJS entry points will always have a default export

danielgindi · 2020-07-19T06:48:46Z

packages/commonjs/test/fixtures/function/dynamic-require-resolve-index/_config.js

@@ -1,4 +1,4 @@
-const nodeResolve = require('@rollup/plugin-node-resolve');
+const { nodeResolve } = require('@rollup/plugin-node-resolve');


Why have you switched to importing the named export in favor of the default export?
Is it just to be explicit or didn't it work anymore with the default export?
I know that it exports both, but we need to be sure that nothing has been fatally broken :-)

This is not an import, this is a CJS require. Since a recent version, this plugin switched to named export mode. We could also have used the default export at .default but then if we need to use a property anyway, I prefer the named export.

danielgindi · 2020-07-19T06:50:24Z

This will be difficult to test with all situations people has experienced out there. Can we tag some people who reported issues so they'll test it live?

danielgindi · 2020-07-19T06:52:15Z

packages/commonjs/test/snapshots/function.js.md

+      ␊
+      };␊
+      ␊
+      module.exports = main;␊


Practically there's no difference here right? As it's an empty object vs the default empty exports object...

Good point. This could be an optimization to add.

Actually, this could be problematic to get rid of because this plugin converts to ESM and that one does not have an implicit default export. So an empty object is generated. I do not see how Rollup core could get rid of this easily later. I think this would be a job to be fixed manually via an exposedExports option.

shellscape

LGTM. As I don't have as deep of insight others I'd like to get buy in from several folks.

lukastaegert · 2020-07-21T04:39:58Z

If anyone is interested: In order to test this, you can clone the repo, run npm link from the packages/commonjs dir and then run npm link @rollup/plugin-commonjs in your test repo. Unfortunately as far as I know, direct Github branch installation does not work for mono repos.

lukastaegert · 2020-07-21T04:41:18Z

I can definitely confirm that this PR solves the issue described in #411. The generated test1.js is now

import test2 from './test2.js';

console.log(test2);
var test1 = 1;

export default test1;

without a reference to __moduleExports, while test2.js remains unmodified.

lukastaegert · 2020-07-21T04:50:00Z

#491 is still broken, but this was also not intended to be covered here. This PR is only about what an ES module receives when importing a CJS module while #491 is about the inverse case.

lukastaegert · 2020-07-21T04:56:23Z

I can confirm that #454 is also resolved by this PR.

lukastaegert · 2020-07-21T05:00:26Z

#431 is still broken. I might have a look at this one.

lukastaegert · 2020-07-21T05:05:50Z

#400 is not fixed, but again this is the same issue as #491.

lukastaegert · 2020-07-21T05:23:30Z

#431 seems to require a lot of setup to reproduce, I guess it should be addressed in a separate PR. So from my side, I still think this one is good to go.

shellscape · 2020-07-21T13:01:52Z

@lukastaegert I can hold off on publishing this if changes for the other issues you mentioned will also be breaking

lukastaegert · 2020-07-21T13:15:44Z

They will, the question is only when they will be ready...

shellscape · 2020-07-21T13:20:34Z

OK. Let's sit on this one for a short while in the event we get a few more in.

guybedford · 2020-07-21T15:06:19Z

FWIW I disagree with this approach since it does not match Node.js interop and likely never will since Node.js support for named exports is now fully stalled after three separate failed initatives over many years.

lukastaegert · 2020-07-21T15:38:29Z

Noted. But at least for bundlers, handling ES modules that were transpiled to CJS in the same way as the original ES module is very important. This PR fixes the default export handling and brings it more in line with how other bundlers handle it. Named exports existed before and after this PR and were not touched here.

guybedford · 2020-07-21T15:43:21Z

It would help to have a flag to change this behaviour for users who want to bundle Node.js apps. I'm sure an issue will come up at some point in the coming months to track this by someone.

lukastaegert · 2020-07-21T15:46:21Z

Sure, as it has been discussed before, and I very much agree we should have it. Forbidding named exports and preventing interop however is only a small part of this, properly handling package.exports and replacing CJS/ESM detection with Node's algorithm would be the bigger one. The main problem is defining a nice way to have the node-resolve and commonjs plugins talk to each other as the former is responsible for package.json information.

lukastaegert · 2020-07-21T15:47:07Z

If we add this, then it should certainly be at least close to the full package, otherwise I am not sure users will see the benefit.

guybedford · 2020-07-21T15:59:56Z

What information from the package is required from the commonjs plugin perspective? It should be just the format info in the ambiguous cases, which could in the worst case be loaded twice with a 15 line package.json boundary check and load and rely on the process-level file system caching of the operating system.

…

On Tue, 21 Jul 2020 at 08:47, Lukas Taegert-Atkinson < ***@***.***> wrote: If we add this, then it should certainly be at least close to the full package, otherwise I am not sure users will see the benefit. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#501 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSX5ITVB6K2IODCDDZLR4W2AZANCNFSM4O6YXUPQ> .

lukastaegert · 2020-07-21T16:04:16Z

Finding and resolving package.json files and in extension libraries relying on them is one of the most performance critical parts of bundling, more performance critical than actually loading the files. The node-resolve plugin has excellent caching to avoid any unnecessary directory lookup while also providing the information which package.json file is responsible for a js file. Would be a waste not to reuse this information.

… from an actual ESM (rollup#501) * fix(commonjs): create more efficient dynamic require code * chore(commonjs): update dependencies BREAKING CHANGES: updates rollup peerDependency to ^2.22.0 * feat(commonjs): support all styles of transpiled ES modules BREAKING CHANGES: CJS entry points will always have a default export

Due to rollup/plugins#501, from version 15.0.0 onwards the generated addon breaks: the rollup plugin automatically creates an `export default` section at the end of the generated code, which the addon system doesn't really like.

lukastaegert mentioned this pull request Jul 17, 2020

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

Closed

lukastaegert marked this pull request as ready for review July 18, 2020 06:12

lukastaegert requested a review from danielgindi July 18, 2020 06:13

lukastaegert marked this pull request as draft July 18, 2020 06:17

lukastaegert marked this pull request as ready for review July 18, 2020 12:28

lukastaegert added 3 commits July 18, 2020 14:40

fix(commonjs): create more efficient dynamic require code

d47f1bd

chore(commonjs): update dependencies

72f84a2

BREAKING CHANGES: updates rollup peerDependency to ^2.22.0

feat(commonjs): support all styles of transpiled ES modules

d8e3a6f

BREAKING CHANGES: CJS entry points will always have a default export

lukastaegert force-pushed the commonjs-better-synthetic-named-exports branch from 149224c to d8e3a6f Compare July 18, 2020 12:57

danielgindi reviewed Jul 19, 2020

View reviewed changes

shellscape changed the title ~~Fix interop when importing transpiled ESM from CJS~~ fix(commonjs): fix interop when importing transpiled ESM from CJS Jul 20, 2020

shellscape approved these changes Jul 20, 2020

View reviewed changes

shellscape requested review from danielgindi, NotWoods, Andarist, guybedford, LarsDenBakker and Rich-Harris July 20, 2020 15:39

lukastaegert mentioned this pull request Jul 21, 2020

[commonjs] cross import between entry commonjs modules is broken #411

Closed

lukastaegert changed the title ~~fix(commonjs): fix interop when importing transpiled ESM from CJS~~ fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module Jul 21, 2020

lukastaegert mentioned this pull request Jul 21, 2020

Regression @rollup/plugin-commonjs v13.0.0 #454

Closed

shellscape merged commit 9ad41a0 into master Jul 21, 2020

shellscape deleted the commonjs-better-synthetic-named-exports branch July 21, 2020 13:00

This was referenced Jul 24, 2020

fix(commonjs): Support __esModule packages with a default export #465

Merged

fix(bundling): downgrade @rollup/plugin-commonjs ionic-team/stencil#2589

Merged

hexetia mentioned this pull request Aug 15, 2020

chore: bump @rollup/plugin-commonjs from 14.0.0 to 15.0.0 vitejs/vite#706

Closed

haoqunjiang mentioned this pull request Sep 4, 2020

Library build target causes the install method to be missing vuejs/vue-cli#5846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501

fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501

lukastaegert commented Jul 17, 2020 •

edited

Loading

lukastaegert commented Jul 18, 2020

lukastaegert commented Jul 18, 2020

lukastaegert commented Jul 18, 2020

danielgindi Jul 19, 2020

lukastaegert Jul 19, 2020

danielgindi commented Jul 19, 2020

danielgindi Jul 19, 2020

lukastaegert Jul 19, 2020

lukastaegert Jul 19, 2020

shellscape left a comment

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

shellscape commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

shellscape commented Jul 21, 2020

guybedford commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

guybedford commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

guybedford commented Jul 21, 2020 via email

lukastaegert commented Jul 21, 2020

		@@ -1,4 +1,4 @@
		const nodeResolve = require('@rollup/plugin-node-resolve');
		const { nodeResolve } = require('@rollup/plugin-node-resolve');

fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501

fix(commonjs): fix interop when importing CJS that is a transpiled ES module from an actual ES module #501

Conversation

lukastaegert commented Jul 17, 2020 • edited Loading

Rollup Plugin Name: commonjs

Description

lukastaegert commented Jul 18, 2020

lukastaegert commented Jul 18, 2020

lukastaegert commented Jul 18, 2020

danielgindi Jul 19, 2020

Choose a reason for hiding this comment

lukastaegert Jul 19, 2020

Choose a reason for hiding this comment

danielgindi commented Jul 19, 2020

danielgindi Jul 19, 2020

Choose a reason for hiding this comment

lukastaegert Jul 19, 2020

Choose a reason for hiding this comment

lukastaegert Jul 19, 2020

Choose a reason for hiding this comment

shellscape left a comment

Choose a reason for hiding this comment

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

shellscape commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

shellscape commented Jul 21, 2020

guybedford commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

guybedford commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 21, 2020

guybedford commented Jul 21, 2020 via email

lukastaegert commented Jul 21, 2020

lukastaegert commented Jul 17, 2020 •

edited

Loading

Rollup Plugin Name: `commonjs`