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

[FredKSchott]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no - I consider this a bugfix, but I'll leave it up to the team how to treat this

List any relevant issue numbers: #266, https://www.pika.dev/npm/snowpack/discuss/367

Description

This fixes #266, and is a follow up to this comment: #451 (comment)

unwrapExports is given the already processed Common.js module, where x.foo === module.exports.foo. We should only need to unwrap further if x was not compiled with __esModule support, in which case everything would still live on the default export. If __esModule exists, there's no need to unwrap.

I can confirm this by removing Object.defineProperty(exports, "__esModule", {value: true}); from is-hotkey in my project, and watching it compile correctly.

My understanding is that this used to be needed for Common.js handling, but as of the latest changes in the latest version of Rollup/this plugin, this function call is no longer needed and is actually problematic, since unwrapping is happening elsewhere (and maybe the unwrap function can even be removed entirely, but would want feedback from a reviewer first)

[FredKSchott]

considering this test as broken as written, and fixed by this PR

[lierdakil]

If I had to guess, #149 is the change that broke this. The caveat is, as far as I can tell, syntheticNamedExports has no provision for both default and named exports in the same module (correct me if I'm wrong).

With unwrapExports, named exports get ignored. In case unwrapExports is removed, named exports will work, but the default export will not. This PR has the same issue.

EDIT: Nope, got it completely wrong. The actual culprit seems to be #427

[lukastaegert]

Looking at this, I wonder if removing unwrapExports altogether is the right move. But would need to check in what situations it actually applies. Before this change, it implemented the interop that extracted the default export from an ES module that was transpiled to CJS.
One question is what happens if a module is converted from ESM to CJS via Rollup and then back via this plugin, will this now preserve all exports?

[lierdakil]

As I said,

With unwrapExports, named exports get ignored. In case unwrapExports is removed, named exports will work, but the default export will not.

That's because rollup will take export default at face value and try to use the exported object as the default export, instead of that object's default property.

So, no, probably not?

This could perhaps be fixed in rollup's core to tell it to look for default export in default property when syntheticNamedExports is true.

[lierdakil]

Here's a quick testcase: https://gist.github.com/lierdakil/8af74b2dc5a5aea76d443803f1b6f902

export default 123;
export const x = 321;

is converted by rollup to CJS as

Object.defineProperty(exports, '__esModule', { value: true });
var test = 123;
const x = 321;
exports.default = test;
exports.x = x;

If we then import this result from another module (both default and x):

import val,{x} from './test-out.js'

console.log('val=', val)
console.log('x=', x)

this all gets bundled into:

// ...
var testOut = createCommonjsModule(function (module, exports) {

Object.defineProperty(exports, '__esModule', { value: true });

var test = 123;
const x = 321;

exports.default = test;
exports.x = x;
});

var val = /*@__PURE__*/unwrapExports(testOut);

console.log('val=', val);
console.log('x=', val.x);

Now, whatever unwrapExports does or does not would be incorrect, the correct version would be

console.log('val=', val.default);
console.log('x=', val.x);

For reference, commonjs-plugin v12 outputs the following:

// ...
var val = unwrapExports(testOut);
var testOut_1 = testOut.x;

console.log('val=', val);
console.log('x=', testOut_1);

which will work correctly.

Side note, v13.0 also mishandles named exported functions: it calls those as a method on module, which binds this to the module instead of binding to current this, i.e. given

const obj = { func: function() { console.log(this); } };

consider the difference between

obj.func();

and

const func = obj.func;
func();

[lukastaegert]

This could perhaps be fixed in rollup's core to tell it to look for default export in default property when syntheticNamedExports is true

I think actually we need a solution along these lines, or maybe a slightly different mode that does this. Will need to think about this a little, hope to find some time in the next days to start writing up a larger sketch of the interop issues and goals we have and how we might to tackle them because a lot of things are interacting here. Meanwhile, I would prefer to put this PR on hold until we have a slightly broader picture.

[lierdakil]

The issue is, commonjs-plugin v13 is kinda-sorta horribly broken, so maybe revert #427 or mark v13 as deprecated on npm while figuring this out?

[shellscape]

The issue is, commonjs-plugin v13 is kinda-sorta horribly broken, so maybe revert #427 or mark v13 as deprecated on npm while figuring this out?

Maybe I'm having an off day, maybe it's a language barrier thing, but I really don't appreciate the snark here. If you truly feel that the version is "horribly broken," you're welcome to use the prior version. Forking is always an option too. Regardless, please dial down the language - it's not helping.

[lierdakil]

To be clear, I did not mean to imply anything about any person with any relation to the project, only that the version in question can cause quite a bit of pain for the end-user. It introduces bugs that are not immediately apparent for reasonably common cases (i.e. "broken"). And it's not immediately obvious that it is doing that, which is, in my opinion, rather horrible.

I don't want to turn this into code of conduct discussion, but I will say that I don't really see the problem. English is not my native language, so some subtleties might elude me. If you want to discuss my use of language further, feel free to e-mail me (the address is in my GitHub profile)

If you truly feel that the version is "horribly broken," you're welcome to use the prior version

I am currently, actually. But I've spent 6 hours debugging this on a new setup before I figured out what's going on and that I need to downgrade. You have to agree, it's far from an ideal user experience. v13 has several issues I've outlined above, so having it as the default for new setups is, let's call it, not optimal. Hence, the suggestion to revert the change or mark the version as deprecated.

[guybedford]

The test case fix seems good, although agreed with @lukastaegert that perhaps removing unwrapExports entirely will be the way to go, since there should be no "default shift" for a CommonJS module loading another CommonJS module with __esModule set.

In the edge case that a CommonJS module has __esModule, default and a named export, the only question is whether import default from 'mod' will return the instance or the default export.

The above question has nothing to do with this function though so removing unwrapExports seems sensible.

[guybedford]

@FredKSchott so I wonder if we could just directly update this PR to entirely remove unwrapExports? Does that expose other compatibility issues?

[FredKSchott]

+1 for removing unwrapExports entirely if others are onboard.

Done in bfc81b4, all tests are passing with snapshots updated (see diff)

[guybedford]

Quite telling that no tests changed... definitely seems like this is mostly vestigal at this point.

[guybedford]

(note this is still a breaking change though and should be a major - so perhaps land this along with any other interop PRs)

[danielgindi]

If you guys want to wait a few days for a major version, so I’ll get a little bit of time on my hands, I want to go over all of the interop code and see if there’s something we can do to satisfy all situations and be consistent.

[FredKSchott]

+1 for doing this the right way and not rushing it as long as there's bandwidth to do so in the near future. If it came down to it, I'd definitely recommend an incremental approach over never doing it at all :)

[FredKSchott]

Bump! We've seen a couple more issues reported regarding this in Snowpack (ie https://www.pika.dev/npm/snowpack/discuss/421) and would love to get it fixed. Let me know if there's anything that I can do to help.

[lukastaegert]

Having created #481 I think this PR is likely a good stop-gap measure until we implemented a proper solution as outlined in #481. I think the only cases that are likely broken after this PR is when importing the default from a CJS module that was a transpiled ES module with named export mode, i.e. when the default export is on the .default property. However I expect this case to be rather rare in the wild especially since Rollup warns you when creating such a package. My recommendation is now to release this as a bug-fix patch release while we work through #481 and make that the next major.

[deavial]

The removal of unwrapExports as a patch no less, is now breaking a bunch of builds. An example of one that breaks with it gone is redux-subscriber.

[TrySound]

PR title is very confusing. Looks like __esModule support is dropped here, not added. Draft-js plugins are broken for me after commonjs plugin upgrade.

[shellscape]

@TrySound sounds like that should be tracked in another issue.

[simonhaenisch]

Yeah I agree with @TrySound that the PR title is confusing. This is more like "break __esModule packages with a default export so that named exports work" 😅 I kind of get the fix but interesting that a fix that's already known to break something else gets so much approval. Luckily there's already #501 which I hope has fixed this 👏

If you truly feel that the version is "horribly broken," you're welcome to use the prior version

I am currently, actually. But I've spent 6 hours debugging this on a new setup before I figured out what's going on and that I need to downgrade. You have to agree, it's far from an ideal user experience. v13 has several issues I've outlined above, so having it as the default for new setups is, let's call it, not optimal. Hence, the suggestion to revert the change or mark the version as deprecated.

Kind of agree with this. Just read the explanation in #497 (comment)... v13.0.0 apparently broke a lot of packages because of unwrapExports so it would make sense to deprecate it. This fix (v13.0.1) removed unwrapExports which broke a few other packages like firebase/messaging, so it would also make sense to deprecate it. But then the latter issue persists for v13.0.2 and v14.0.0 as well, so you'd need to deprecate them as well, and that sounds a bit excessive 😅

Deprecation doesn't mean that the version isn't installable anymore btw, just that you'll be able to show a deprecation message to devs (see docs), like

npm deprecate @rollup/[email protected] "This version breaks the default export of __esModule packages."

Could save some devs some time, or we just hope they'll find this thread 😄

BTW we've ran into this with @stencil/core which uses rollup to bundle the user's code, and therefore @rollup/plugin-commonjs is not a visible dependency to the end user anymore. Versions 1.16.x and 1.17.0 of stencil are broken when using a dependency that's ESM with default export -> CJS -> imported as ESM (pretty sure that's exactly what #501 fixed).

/cc @bitflower who had issues with stencil + feathers-reduxify-authentication because of this.

[shellscape]

For the sake of full disclosure; I've deleted the comment by @lierdakil. It was, in every sense of the term, off-topic. The issues and PR threads on the repository are not a forum for airing grievances, proselytizing, rants, nor for declaring disdain or departure from the project. If during an unfortunate set of events one chooses to move on from Rollup, please do so without posting here. There are many other forums and mediums for doing so that are far more effective.

As this PR has gone off the rails, it's been locked from further comments. If you have an issue or bug that has not been addressed by other issues, please do open a new issue.

The https://liberamanifesto.com/ is always a good read.