CommonJs interop consistency

[sokra]

• Rollup Plugin Name: @rollup/plugin-commonjs
• Rollup Plugin Version: 16.0.0
• Rollup Version: 2.33.3
• Operating System (or Browser): Windows
• Node Version: 15.2.1
• Link to reproduction (⚠️ read below): https://github.com/sokra/interop-test https://github.com/sokra/interop-test/tree/main/modules

Expected Behavior

Being consistent within itself. I would expect that these code snippets give the similar results:

• import x
• import { default as x }
• import * as x; x.default
• import * as x; ident(x).default (ident is an identity function, e. g. Object())

And also

• import { named as x }
• import * as x; x.named
• import * as x; ident(x).named (ident is an identity function, e. g. Object())

And also

• import { __esModule as x }
• import * as x; x.__esModule
• import * as x; ident(x).__esModule (ident is an identity function, e. g. Object())

Actual Behavior

https://sokra.github.io/interop-test/#rollup

See fixture sources: https://github.com/sokra/interop-test/tree/main/modules

The link also contains comparisons against other tools, so you might take a look that this to see how other tools behave for a specific test case:

e. g. named-and-default-export-esModule-esm-reexport

or for a specific syntax.

See https://sokra.github.io/interop-test/ for explanations.

Additional Information

cc @guybedford @lukastaegert

[shellscape]

Thanks for going the extra mile and posting an issue. We have been discussing this internally when your comparisons went public. To keep things tidy, I'm closing this in favor of tracking in #481 and will post a comment there back to this issue for cross reference.

[guybedford]

@sokra note that there is a significant refactoring in progress to the CommonJS plugin in #537.

That PR will be released soon. I believe that should fix a lot of these cases, as per the bugs in the PR linked by @shellscape. If you're able to run tests against that, that would be a huge help. Alternatively it would be good to ensure the compat tables are updated when that comes out. Thanks again for doing this analysis.

[sokra]

Sure. Here is the diff the PR causes to the rollup chart:

rollup-rollup-pr	fixture	rollup	rollup-pr
import x import { default as x } import * as x; x.default import * as x; ident(x).default	named-and-default-export-duplicate	compilation error	{ named, default }
import x import { default as x } import * as x; x.default import * as x; ident(x).default	named-and-default-export-esModule-reexport	{ [__esModule], named, default }	{ named, default, [__esModule] }
import x import { default as x } import * as x; x.default import * as x; ident(x).default	named-export-esModule	{ [__esModule], named }	{ named, [__esModule] }
import x import { default as x } import * as x; x.default import * as x; ident(x).default	order-esModule	{ [__esModule], b, a, c }	{ b, a, c, [__esModule] }
import { named as x }	named-and-default-export-duplicate named-and-default-export-esModule-esm-reexport	compilation error	'named'
import * as x; x.named import * as x; ident(x).named	named-and-default-export-duplicate	compilation error	'named'
import * as x; x.named	named-and-default-export-esModule-esm-reexport	undefined + warnings	'named'
import { __esModule as x } import * as x; x.__esModule import * as x; ident(x).__esModule	named-and-default-export-duplicate	compilation error	undefined
import * as x	default-export-esModule-esm-reexport	[Object: null prototype] { default, __moduleExports: { [__esModule], default } }	[Object: null prototype] { default, __moduleExports: { default, [__esModule] } }
import * as x import()	named-and-default-export-duplicate	compilation error	[Object: null prototype] { named, default: { named, default } }
import * as x	named-and-default-export-esModule-esm-reexport	[Object: null prototype] { named, default, __moduleExports: { [__esModule], named, default } }	[Object: null prototype] { named, default, __moduleExports: { named, default, [__esModule] } }
import * as x import()	named-and-default-export-esModule-reexport	[Object: null prototype] { named, default: { [__esModule], named, default } }	[Object: null prototype] { named, default: { named, default, [__esModule] } }
import * as x import()	named-export-esModule	[Object: null prototype] { named, default: { [__esModule], named } }	[Object: null prototype] { named, default: { named, [__esModule] } }
import * as x import()	order-esModule	[Object: null prototype] { b, a, c, default: { [__esModule], b, a, c } }	[Object: null prototype] { b, a, c, default: { b, a, c, [__esModule] } }
import()	default-export-esModule-esm-reexport	{ __moduleExports: { [__esModule], default } }	{ __moduleExports: { default, [__esModule] } }
import()	named-and-default-export-esModule-esm-reexport	{ __moduleExports: { [__esModule], named, default } }	{ __moduleExports: { named, default, [__esModule] }, named }

That would be to full chart:

rollup	import x import { default as x }	import * as x; x.default	import * as x; ident(x).default	import { named as x }	import * as x; x.named	import * as x; ident(x).named	import { __esModule as x }	import * as x; x.__esModule	import * as x; ident(x).__esModule	import * as x	import()
default-export default-export-runtime	{ default }	{ default }	{ default }	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { default: { default } }	[Object: null prototype] { default: { default } }
default-export-esModule	'default'	'default'	'default'	undefined	undefined	undefined	true	true	undefined	[Object: null prototype] { default }	[Object: null prototype] { default }
default-export-esModule-esm-reexport	compilation error	undefined + warnings	'default'	compilation error	undefined + warnings	undefined	compilation error	undefined + warnings	undefined	[Object: null prototype] { default, __moduleExports: { default, [__esModule] } }	{ __moduleExports: { default, [__esModule] } }
default-export-esm	'default'	'default'	'default'	compilation error	undefined + warnings	undefined	compilation error	undefined + warnings	undefined	[Object: null prototype] { default }	{ default }
named-and-default-export named-and-default-export-duplicate named-and-default-export-reexport named-and-default-export-runtime single-object-with-default-export single-object-with-default-export-duplicate	{ named, default }	{ named, default }	{ named, default }	'named'	'named'	'named'	undefined	undefined	undefined	[Object: null prototype] { named, default: { named, default } }	[Object: null prototype] { named, default: { named, default } }
named-and-default-export-babel-getter named-and-default-export-esModule named-and-default-export-esModule-duplicate named-and-default-export-runtime-esModule	'default'	'default'	'default'	'named'	'named'	'named'	true	true	undefined	[Object: null prototype] { named, default }	[Object: null prototype] { named, default }
named-and-default-export-esModule-esm-reexport	compilation error	undefined + warnings	'default'	'named'	'named'	'named'	compilation error	undefined + warnings	undefined	[Object: null prototype] { named, default, __moduleExports: { named, default, [__esModule] } }	{ __moduleExports: { named, default, [__esModule] }, named }
named-and-default-export-esModule-reexport	{ named, default, [__esModule] }	{ named, default, [__esModule] }	{ named, default, [__esModule] }	'named'	'named'	'named'	true	true	undefined	[Object: null prototype] { named, default: { named, default, [__esModule] } }	[Object: null prototype] { named, default: { named, default, [__esModule] } }
named-and-default-export-esm	'default'	'default'	'default'	'named'	'named'	'named'	compilation error	undefined + warnings	undefined	[Object: null prototype] { named, default }	{ default, named }
named-and-default-export-esm-reexport	compilation error	undefined + warnings	'default'	'named'	'named'	'named'	compilation error	undefined + warnings	undefined	[Object: null prototype] { named, default, __moduleExports: { named, default } }	{ __moduleExports: { named, default }, named }
named-and-default-export-getter	{ [named]: [G], [default]: [G] }	{ [named]: [G], [default]: [G] }	{ [named]: [G], [default]: [G] }	'named'	'named'	undefined	undefined	undefined	undefined	[Object: null prototype] { default: { [named]: [G], [default]: [G] } }	[Object: null prototype] { default: { [named]: [G], [default]: [G] } }
named-and-default-export-getter-esModule	'default'	'default'	'default'	'named'	'named'	undefined	true	true	undefined	[Object: null prototype] { default }	[Object: null prototype] { default }
named-and-default-export-inherited	{ named, default }	{ named, default }	{ named, default }	'named'	'named'	undefined	undefined	undefined	undefined	[Object: null prototype] { default: { named, default } }	[Object: null prototype] { default: { named, default } }
named-and-default-export-live	{ named, default }	{ named, default }	{ named, default }	'named'	'named'	'named-outdated'	undefined	undefined	undefined	[Object: null prototype] { named: 'named-outdated', default: { named, default } }	[Object: null prototype] { named: 'named-outdated', default: { named, default } }
named-and-default-export-non-enumerable named-and-default-export-non-enumerable-inherited	{ [named], [default] }	{ [named], [default] }	{ [named], [default] }	'named'	'named'	undefined	undefined	undefined	undefined	[Object: null prototype] { default: { [named], [default] } }	[Object: null prototype] { default: { [named], [default] } }
named-and-null-default-export named-and-null-default-export-runtime single-object-with-null-default-export	{ named, default: null }	{ named, default: null }	{ named, default: null }	'named'	'named'	'named'	undefined	undefined	undefined	[Object: null prototype] { named, default: { named, default: null } }	[Object: null prototype] { named, default: { named, default: null } }
named-and-null-default-export-esModule named-and-null-default-export-runtime-esModule	null	null	null	'named'	'named'	'named'	true	true	undefined	[Object: null prototype] { named, default: null }	[Object: null prototype] { named, default: null }
named-and-null-default-export-non-enumerable	{ [named], [default]: null }	{ [named], [default]: null }	{ [named], [default]: null }	'named'	'named'	undefined	undefined	undefined	undefined	[Object: null prototype] { default: { [named], [default]: null } }	[Object: null prototype] { default: { [named], [default]: null } }
named-export named-export-runtime single-object-export	{ named }	{ named }	{ named }	'named'	'named'	'named'	undefined	undefined	undefined	[Object: null prototype] { named, default: { named } }	[Object: null prototype] { named, default: { named } }
named-export-esModule	{ named, [__esModule] }	{ named, [__esModule] }	{ named, [__esModule] }	'named'	'named'	'named'	true	true	undefined	[Object: null prototype] { named, default: { named, [__esModule] } }	[Object: null prototype] { named, default: { named, [__esModule] } }
named-export-esm	compilation error	undefined + warnings	undefined	'named'	'named'	'named'	compilation error	undefined + warnings	undefined	[Object: null prototype] { named }	{ named }
named-export-non-enumerable	{ [named] }	{ [named] }	{ [named] }	'named'	'named'	undefined	undefined	undefined	undefined	[Object: null prototype] { default: { [named] } }	[Object: null prototype] { default: { [named] } }
named-export-runtime-esModule	{ [__esModule], named }	{ [__esModule], named }	{ [__esModule], named }	'named'	'named'	'named'	true	true	undefined	[Object: null prototype] { named, default: { [__esModule], named } }	[Object: null prototype] { named, default: { [__esModule], named } }
order	{ b, a, c }	{ b, a, c }	{ b, a, c }	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { b, a, c, default: { b, a, c } }	[Object: null prototype] { b, a, c, default: { b, a, c } }
order-esModule	{ b, a, c, [__esModule] }	{ b, a, c, [__esModule] }	{ b, a, c, [__esModule] }	undefined	undefined	undefined	true	true	undefined	[Object: null prototype] { b, a, c, default: { b, a, c, [__esModule] } }	[Object: null prototype] { b, a, c, default: { b, a, c, [__esModule] } }
order-esm	compilation error	undefined + warnings	undefined	compilation error	undefined + warnings	undefined	compilation error	undefined + warnings	undefined	[Object: null prototype] { b, a, c }	{ a, b, c }
single-empty-string-export	''	''	''	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { default: '' }	[Object: null prototype] { default: '' }
single-null-export	null	null	null	type error	type error	undefined	type error	type error	undefined	[Object: null prototype] { default: null }	[Object: null prototype] { default: null }
single-promise-object-export	Promise { { named } }	Promise { { named } }	Promise { { named } }	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { default: Promise { { named } } }	[Object: null prototype] { default: Promise { { named } } }
single-promise-object-with-default-export	Promise { { named, default } }	Promise { { named, default } }	Promise { { named, default } }	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { default: Promise { { named, default } } }	[Object: null prototype] { default: Promise { { named, default } } }
single-promise-string-export	Promise { 'single' }	Promise { 'single' }	Promise { 'single' }	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { default: Promise { 'single' } }	[Object: null prototype] { default: Promise { 'single' } }
single-string-export single-string-export-defined single-string-export-duplicate single-string-export-getter single-string-export-reexport	'single'	'single'	'single'	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { '0': 's', '1': 'i', '2': 'n', '3': 'g', '4': 'l', '5': 'e', default: 'single' }	[Object: null prototype] { '0': 's', '1': 'i', '2': 'n', '3': 'g', '4': 'l', '5': 'e', default: 'single' }
single-string-export-esm-reexport	compilation error	undefined + warnings	undefined	compilation error	undefined + warnings	undefined	compilation error	undefined + warnings	undefined	[Object: null prototype] { '0': 's', '1': 'i', '2': 'n', '3': 'g', '4': 'l', '5': 'e', __moduleExports: 'single' }	{ __moduleExports: 'single' }
single-string-export-live	'single-outdated'	'single-outdated'	'single-outdated'	undefined	undefined	undefined	undefined	undefined	undefined	[Object: null prototype] { '0': 's', '1': 'i', '2': 'n', '3': 'g', '4': 'l', '5': 'e', '6': '-', '7': 'o', '8': 'u', '9': 't', '10': 'd', '11': 'a', '12': 't', '13': 'e', '14': 'd', default: 'single-outdated' }	[Object: null prototype] { '0': 's', '1': 'i', '2': 'n', '3': 'g', '4': 'l', '5': 'e', '6': '-', '7': 'o', '8': 'u', '9': 't', '10': 'd', '11': 'a', '12': 't', '13': 'e', '14': 'd', default: 'single-outdated' }

I'll update the tables once it's released, but the PR doesn't seem to adress as many issues as you hope...

[guybedford]

@sokra the first list seems like quite a few fixes to me. Do you mean that there are other cases in the full chart not addressed?

Could you just copy the exact cases you are concerned about that are still missing? Perhaps we can open a new issue to discuss those new cases specifically, and that would definitely be a help to cross-check before this release goes out.

[sokra]

Could you just copy the exact cases you are concerned about that are still missing?

I really would to see import * as x; ident(x).default with *-esm-reexport getting fixed. A export * should never reexport the default export.

[guybedford]

Thanks... is that the only case that sticks out here? I can post a new issue for that separately then.

[sokra]

I also wondered about import * as x; ident(x).__esModule being always undefined. I would have done that different, but this seem to be an intentional choice. Babel, esbuild and webpack have this always true. Node.js has this true when the module defines exports.__esModule.

Another notable thing is that the namespace object (import * as x; ident(x).named) doesn't expose non-enumerable properties, but direct importing them is possible (import * as x; x.named).

Is also notable that direct imports (import * as x; x.named) are live-bindings, while indirect imports (import * as x; ident(x).named) are not (they copy their value after evaluation).

Another notable thing is that modules with __esModule get the default property as default export, but not when reexporting them via module.exports = require("module"). Similar thing happens when __esModule flag is not statically detectable, e. g. when using

[guybedford]

@sokra thanks for the summary here.

Did you mean to finish with an example like the following?

function setEsInterop (exports) {
  Object.defineProperty(exports, "__esModule", { value: true });
}
setEsInterop(exports);

[sokra]

Oh yes, I was interrupted and thought this was done. There are many possible ways to do that...

[guybedford]

It's worth noting that Node.js won't support these cases either.

[sokra]

Not supporting would be fine too. I only concerned about inconsistencies within rollup itself.

[sokra]

It's also worth noting that most of these cases are edge cases. So if rollup chooses to not fix that that's not super concerning. I just want to report these things so you are aware of them, and maybe they are easily fixable.

[Aghassi]

I'm sorry to comment on a closed issue, but I have to flag this as one of the worst kinds of issues to debug as a developer.

If it weren't for vitejs/vite#2139 (comment), I would not have found a solution. It's not even a nice solution, it's just one that works.

I had to jump from that issue to this one #481, to here. Only to find that rollup is basically not aligned with spec. As a developer, I can't control every problem in the ecosystem, but this sort of interop one is the worst kind because it makes it my problem when there are very real syntax changes as part of migrating to things like ESM. It also holds back the ecosystem because people may not make the jump if they can't debug why something is failing when it wasn't before.

It would make a big difference if this edge case could be fixed specifically because things like Vite use rollup. Everything in the Webpack ecosystem is basically protected from this sort of issue and it makes me inclined to recommend Sokra's tooling over rollup because I can trust it.

And to the point that Node won't support this use case, that's fine. Once stuff is all ESM it doesn't really matter. It's specifically for being able to move modules to ESM which may depend on module that say ship their code as bundled instead of as source code. The goal, once on ESM, is to do bundleless development anyway.

If there is anything I can do personally to aid in this improvement being made to the tooling, please let me know.

[shellscape]

Issues in general (especially closed issues) aren't the proper place to vent frustrations. There are social channels for that, and unfortunately the majority of the reply doesn't add anything of value to the conversation. You've encountered an edge case, and while every developer feels empathy for that, we also acknowledge what that means.

it makes me inclined to recommend Sokra's tooling over rollup because I can trust it.

This is never a compelling bit to add to an issue. It won't motivate the way you might think it will. Recommending other tooling does not affect the project or the maintainers, we're not in competition for users with any other project, and you are free to do so. In fact, if that helps you or your colleagues in your endeavors we'd endorse using different tooling.

If there is anything I can do personally to aid in this improvement being made to the tooling, please let me know.

#481 is the place for you. The Rollup project operates the same as Vite - community contributions. We're woefully short on maintainer time and have been for some time. Very few people step up to help with tooling. If you'd like to aid in improving the commonjs plugin, please review the outstanding items for #481 and contribute pull requests. Or you could always convince Robinhood to donate a substantial amount of cash for Lukas to take time off of work to complete the items on #481.