feature(commonjs): support for destructuring require for named imports

[danielgindi]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

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

List any relevant issue numbers:

#400 - Kind of relevant but not really. I mean it won't solve the issue unless they move to named imports anyway.

Description

This PR adds detection of "named imports" in CJS, which do not really exist but as people are starting to fake it with object destructuring it becomes relevant.

So what happens now is that if you are using object destructuring syntax with require, it tries to detect whether there are named exports in the imported module.

• If it does - then it imports all of those, with an extra default in case you import default.
• If it does not - then it imports the default export as usual, and it will destructure that as usual.

All existing tests pass without any change to snapshots or configuration, and the new tests are passing with flying colors for both CJS->CJS and EJS->CJS.

Theoretical breaking changes

In theory, if someone has existing code where an ES-compatible CJS module is imported AND it has both default and named exports, AND they are using object destructuring in the require statement - the behavior will change.
But in every such module I've seen where CJS compatibility is required, people exported the "named exports" over the default object too.

[LarsDenBakker]

So this means I might have different behavior between these:

const { foo }  = require('bar');

console.log(foo);

and

const bar = require('bar');

console.log(bar.foo);

How do other tools deal with this?

[danielgindi]

@LarsDenBakker They don't. It's just that in modules that are designed to be compatible with CJS, they add the named exports as members of the default export, or omit the default export. In those cases it will keep the same behavior.
But if the module being imported is a strict ES module, which has actual named/default exports and not module.exports, then we detect it and behave accordingly.

You can refer to the discussion in #400 and refer to the links I put there.

[Andarist]

I don't know - this IMHO is a dangerous thing to do. While it can improve some scenarios it can lead to obscure bugs in others - I certainly wouldn't like to bump into an issue caused by this as a developer.

The node ecosystem is pushing towards very strict rules in terms of CJS and ESM interoperability and I feel that introducing yet-another-non-strict semantic to the ecosystem at this point in time shouldn't be discouraged. The issue presented by @LarsDenBakker is IMHO a very good argument against this.

[danielgindi]

@Andarist please note that this only affect how you import named exports from ESM and not from CJS.
As for importing from CJS there is no solution.

I have yet to see a situation where this would break things with some module.

• In most cases people use destructuring when requiring modules only when instructed to do so by the module author, which would mean that the author generate compatibility exports for CJS too. Which means it would work either way.
• In case you do regular require (no destructuring)- the behaviour stays the
  same.

[Andarist]

I'm worried that this might affect existing packages that destructure from the default export. The problem is not even that this might affect such packages but it might also affect their, unaware, consumers.

Could you describe in detail what exact mix of files is this fixing?

[danielgindi]

This affects only the situation where a commonjs ‘require’s an ES module, and that ES module has named exports.

Let’s say that there’s a CJS module that requires an ES module.
Now there could be several situations:

1. ES module builds “named exports” as members of the default export for CJS compat -> if the cjs module uses destructuring to get default export, behaviour stays the same. / if the cjs module destructures directly to named imports, then it currently does not work, but after this PR it will work.
2. ES module does not have special CJS compat -> currently the cjs module can’t destructure-require, this PR fixes it.
3. The ES module is actually compiled for CJS -> nothing changes, as this only affects cases with actual ES modules.

[shellscape]

@LarsDenBakker @lukastaegert thoughts on the latest round of comments?

[FredKSchott]

what if a default-style require call lives here together:

const { a, b } = require('./foo.js');
const def = require('./foo.js');

t.is(a, 1);
t.is(b, 2);
t.is(def, ???);

It would be good to test this since it feels undefined at the moment.

[guybedford]

This seems in contrast to the interop implemented in Webpack right? Surely matching Webpack should be a goal to at least offer some consistency here?

Consumer-driven interpretation does seem risky to me.

[lukastaegert]

There is one issue here that should be considered: Replacing a destructuring import with a named import will create a live-binding where before there was none. On the other hand, I expect

• for the first time, actual tree-shaking between CJS modules
• more optimized code

I hope by tomorrow I might be able to post some output examples because I think that is what is missing here to advance the discussion. Not sure what to do about the live-binding. If this really turns out to be an issue, we should add a flag to deactivate this.

This seems in contrast to the interop implemented in Webpack right? Surely matching Webpack should be a goal to at least offer some consistency here?

What do you mean by "matching Webpack"? Do you have examples of what code Webpack generates and what code this PR creates instead, otherwise I feel this is not really helping the discussion but creating churn. Also I tend to state that "matching Webpack" is not a goal, working output is, and if we can create more efficient output that does the same, we should go for it.

[guybedford]

I think this is more about the fact that the require() of an ES module should return the ES module namespace object, and not have any other tricks involved than that.

Webpack is only used as an example, the issue specifically being that following patterns that are "unique" will only lead to compatibility woes when transitioning between tools.

RollupJS can certainly make the decision to do that, but this concern is certainly a factor in making that decision.

[lukastaegert]

These generated named exports are not visible to the user and just used for internal optimization. If it behaves the same, there should be no concerns. With the same argument you could say a bundler should not put any modules into the same files because then there are tricks involved. This plugin is already doing a ton of tricks, which even involve scope-hoisting CJS modules that are written in a compatible way. If something is actually not working as expected with this change, please point out what specifically that is, and ideally by pointing at the generated code.

[guybedford]

I don't personally use this plugin but am following simply because I was cc'd and care about cross-tooling interop especially since it affects all of us through usage. So please excuse if I don't have all the context here! From looking a bit more closely it seems like currently require('esm') will return the default export? If so, then it seems like it is the existing behaviour that is actually problematic and this PR is a step (but not far enough) in the right direction.

require('esm') for export default x should always require users to access require('esm').default and that this isn't already the case seems like an issue?

I believe this is what Webpack does, but perhaps @sokra can clarify on the interop here.

[sokra]

I believe this is what Webpack does

Yep, in webpack require("esm") returns the namespace object of the module.

[danielgindi]

My two cents are that webpack’s way of doing things aren’t perfect either.

1. It’s not that “natural” to take require('esm').default
2. It creates compatibility issues when someone decides to add an ES dist version and a module member to their package.json (which means webpack is already incompatible with itself. Why should we fear being incompatible with it?)
3. It forces people to think too much about those imports.

Besides, module authors usually go the extra mile to make sure the package will work for everyone. This includes adding named exports as members of the default exports. And this is the only case I’ve ever seen anyone destructuring a require.

The popular npm package uuid is an example of that, and its README is also instructing people to use destructuring.

[danielgindi]

require('esm') for export default x should always require users to access require('esm').default and that this isn't already the case seems like an issue?

I don’t know if that’s an issue. But if we want to improve compatibility with webpack we could detect the usage of require('esm').default easily. Of course if the default export has a member named default then that’s an ambiguity that we will break.

But if you’re destructuring const { default } = require('esm'); then this PR will also solve it and make it import default as expected.

[danielgindi]

There is one issue here that should be considered: Replacing a destructuring import with a named import will create a live-binding where before there was none. On the other hand, I expect

What do you here mean by “live-binding”?

[guybedford]

One of the goals of RollupJS is to follow the language specification itself.

Under this transformation, I believe the following invariant no longer holds:

const m = require('mod');
const { name } = m;

being equivalent to:

const { name } = require('mod');

Perhaps we can focus on this deviation from the spec as being at the root of the concern then?

[Andarist]

What do you here mean by “live-binding”?

export let foo = 1
export function changeFoo() { foo = 2 }

The change of foo is reflected in the importing module as this is a live binding. When u destructure a thing you „disconnect” it from having this property of being a live binding, it is only read at assignment time in the requiring file.

[lukastaegert]

I do not really understand. My expectation was that with this PR, the destructuring would be replaced by direct variable accesses, and the reified namespace object would not really be needed. But in all test cases, I still see the destructuring. And I fail to generate a test case where anything is improved. How come?

[danielgindi]

Were you expecting rollup itself to optimize this case or the plugin?

[lukastaegert]

I was expecting the plugin create matching named exports and imports and replace the destructuring with those imports. Then Rollup would see no reason to create the namespace object.

At the moment, it is not really clear to me what this PR actually changes in terms of generated code. Could you give some examples with input code, generated output without this PR, generated output with this PR to get an idea of what this PR actually does?

[lukastaegert]

Ah, I think I understand now what I misunderstood.

[lukastaegert]

So what this really does is if there are named exports,

const { foo } = require('bar');

no longer becomes

import bar from 'bar';
const { foo } = bar;

but rather

import * as bar from 'bar';
const { foo } = bar;

Is that essentially correct?

[lukastaegert]

Interestingly I stumbled upon a bug in our logic here that predates your PR:

If I write

exports.foo = 42;
exports.foo = 43;

in my imported file, Rollup throws a "Duplicate export 'foo'" error. Alas...

[lukastaegert]

Except that I do not get it to work, see my other comment, my feeling is that this is one of the "95% of the users will benefit from this, but it could cause issues for 5% because it changes semantics" things. Not sure how we should handle this, the benefits of such a feature could be tremendous in terms of generated code quality and size.

[danielgindi]

Except that I do not get it to work, see my other comment, my feeling is that this is one of the "95% of the users will benefit from this, but it could cause issues for 5% because it changes semantics" things. Not sure how we should handle this, the benefits of such a feature could be tremendous in terms of generated code quality and size.

This PR is not sacred, it does not fix a bug but improves semantics of interop with ESM.
If we get to the conclusion that it could have negative impact, then let’s not merge.
Problem is I only have a theoretic case to when it will harm users, but haven’t seen it in practice.

There is also the language semantics issue (mentioned by @guybedford).
Which I’m conflicted about, since the ES import/export itself works this way. And CJS has no spec (that I know of) of how it should work when requiring ESM.

[Andarist]

It creates compatibility issues when someone decides to add an ES dist version and a module member to their package.json (which means webpack is already incompatible with itself. Why should we fear being incompatible with it?)

Could you elaborate o this one? Note: adding "module" is often a breaking change but people rarely recognize it being one.

Besides, module authors usually go the extra mile to make sure the package will work for everyone.

I can't say I agree with this statement - IMHO usually, people have no idea how to get all of this "right" as this stuff is overly complicated in practice and it's hard to expect from module authors to know all intricacies related to this upfront

This includes adding named exports as members of the default exports.

This happens, but I wouldn't say that this is super common and, on a personal level, I would advise against doing this, ever. It creates false sense of how things are supposed to work, creates multiple ways of doing something and very often prevents tree-shaking from working correctly.

---

General note from me - I'm just really worried that this will make Rollup working differently than webpack (and yes, IMHO this is a problem) and other tools. The problem is not that this would work differently per se, but it's a problem for consumers, transitive dependencies, etc because a particular library could work when bundled with one tool but wouldn't work when bundled with other. And this affects not only module authors, but more importantly - regular users, who most of the time have no idea about all of this CJS vs ESM interop stuff and can trip over things like this badly. Interop in the community is very important from my point of view.

[danielgindi]

Note: adding "module" is often a breaking change but people rarely recognize it being one.

As you said, they rarely recognize it being a breaking change. This is an issue.

I can't say I agree with this statement - IMHO usually, people have no idea how to get all of this "right" as this stuff is overly complicated in practice and it's hard to expect from module authors to know all intricacies related to this upfront

I guess it depends on which modules you’ve used I guess.

This happens, but I wouldn't say that this is super common and, on a personal level, I would advise against doing this, ever. It creates false sense of how things are supposed to work, creates multiple ways of doing something and very often prevents tree-shaking from working correctly.

I don’t have anything against it, more than any other backwards-compat trick out there. It’s all about an author trying to provide compatibility.

General note from me - I'm just really worried that this will make Rollup working differently than webpack (and yes, IMHO this is a problem) and other tools. The problem is not that this would work differently per se, but it's a problem for consumers, transitive dependencies, etc because a particular library could work when bundled with one tool but wouldn't work when bundled with other. And this affects not only module authors, but more importantly - regular users, who most of the time have no idea about all of this CJS vs ESM interop stuff and can trip over things like this badly. Interop in the community is very important from my point of view.

I do not have opinion on this matter. When I first switched to rollup, it was nearly impossible to roll most cjs packages anyway.
After reading a bit, I found out that rollup is about static imports, being able to do real optimizations and tree-shaking in compile time.
Of course I then added a feature to this plugin to be able to support dynamic requires. This was keeping in mind that everything is shifting towards ES imports.

[FredKSchott]

I played around with this problem a bit today for Snowpack, and appreciate what a tough problem this is. If I understand correctly, in Rollup a CJS require() call should be compiled to a default import because each CJS module is effectively compiled to a single default export. This works for many cases but breaks the case outlined in #400 (CJS -> ESM).

I was going to recommend that this plugin move closer to Webpack's behavior mentioned above and convert any require() call to return the full namespace IF the required module is an ESM module (keep existing behavior if it resolves to CJS). We could also add a quick check that the required module is never called as a function as well, just to be safe.

I have a demo of this working for Snowpack if anyone is interested, but it requires the user to send the plugin a list of known ESM modules ahead of time. This works for our usecase, but wouldn't make sense for most users.

Does a plugin have a way to check whether an imported dependency is ESM or CJS? If that was possible, I could get rid of that odd workaround and have something worth submitting for PR.

[lukastaegert]

Does a plugin have a way to check whether an imported dependency is ESM or CJS?

This is not simple, but has been done before, basically by this plugin. What it does is to add a transform hook that first scans each file for its format to build a table. This is done in two passes: First a simple string matching to look for CJS keywords module, exports, require. If none are found, we assume it is ESM. Otherwise we actually this.parse to an AST and look for import and export statements, if none are found we assume it is CJS (at least I think that is roughly how it is done).

Now if we want to know if an import is CJS or ESM, we postpone the hook that wants to know that until the corresponding table entry has been generated.

There is just one downside: When you do this in a setup where you also have the commonjs plugin, the CJS detection would need to run BEFORE the commonjs plugin, while your import resolution would likely want to run AFTER the commonjs plugin. So basically you would need two plugins. Maybe it would be simpler to make the table built by this plugin available to other plugins...

[lukastaegert]

Regarding this PR, my feeling at the moment is that I would rather not have special handling of the import based on the use of destructuring. Instead, we should try to fix interop as a whole. There are two things to do. The simple one is to align importing ESM with how Rollup's "auto" export mode works https://rollupjs.org/guide/en/#outputexports:

• If there is a single default export AND NO NAMED EXPORTS, then it assigns the default to module.exports
• In all other cases (even if there is a default export), it assigns all exports to the exports object, even the default becomes exports.default.

So that means in inverse, a require should ONLY return the default export if it exists and THERE ARE NO NAMED EXPORTS, otherwise it should return the namespace. This should have been fixed a long time ago.

On a larger scale, though, I think we should eventually switch to always using the namespace by default, because that is what mostly everyone else (Webpack, Babel, TypeScript) apparently is doing. As there are many packages that may break by this, though, there should be an easy way to list packages where instead we want require to return the default.

[Andarist]

On a larger scale, though, I think we should eventually switch to always using the namespace by default, because that is what mostly everyone else (Webpack, Babel, TypeScript) apparently is doing.

What would that mean for the exports: 'auto'? Other tools don't provide something like this and its existence breaks community interop in the exact situation you have mentioned - when a package has a default export and no named exports, see webpack/webpack#7973

[lukastaegert]

It will not go away. It is the only way to create a CJS package that exports one thing via module.exports from an ESM source. Maybe there will be a warning to prompt you to make your choice explicit that tells you about the consequences.

[Andarist]

So if you go with "always using the namespace by default" in here wont keeping exports: 'auto' amplify the problem outlined in the linked issue for your consumers? Like - the problem is still there now, but it's affecting "only" other bundlers.

It seems really that those 2 semantics are fighting with each other badly so having them both as defaults like this - even just keeping exports: 'auto' as opt-in would fight against the proposed semantic of this plugin ("always using the namespace by default"). From my POV this is a usability problem as the Rollup's ecosystem would not work in a cohesive way.

[danielgindi]

Regarding this PR, my feeling at the moment is that I would rather not have special handling of the import based on the use of destructuring. Instead, we should try to fix interop as a whole. There are two things to do. The simple one is to align importing ESM with how Rollup's "auto" export mode works https://rollupjs.org/guide/en/#outputexports:

• If there is a single default export AND NO NAMED EXPORTS, then it assigns the default to module.exports
• In all other cases (even if there is a default export), it assigns all exports to the exports object, even the default becomes exports.default.

So that means in inverse, a require should ONLY return the default export if it exists and THERE ARE NO NAMED EXPORTS, otherwise it should return the namespace. This should have been fixed a long time ago.

On a larger scale, though, I think we should eventually switch to always using the namespace by default, because that is what mostly everyone else (Webpack, Babel, TypeScript) apparently is doing. As there are many packages that may break by this, though, there should be an easy way to list packages where instead we want require to return the default.

Actually, doing that for format: cjs outputs - will apply a standard that will effectively remove issue #411 too. It's probably the right solution, if we couldn't find any solution for #411 that will actually work.

[lukastaegert]

@Andarist The problem is the following:
If you convert a CJS file that just assigns a value to module.exports to ESM, what would you expect to get as an export? The only answer that makes sense here is a single default export. auto mode is meant to invert this operation, so converting a file that just has a single default export to CJS produces a file that assigns this value to module.exports. If we instead converted the file to something that assigns the value to exports.default feels much more inconsistent to me.

But that is not the point to be discussed here, the point is that there is demand for being able to create such CJS files and taking this ability away from our users seems stupid to me, or rather like trying to force a war "either assign your export to a key of exports or do not use Rollup". This is like Webpack's stance on the matter.

So I would rather see Rollup's point to educate users about the consequences of their choice. If the library does not have an ESM output but just CJS, there is nothing wrong with "auto" mode. And if they want to change, maybe it should be their choice if and when to do a breaking change release.

Also, there is no "rollup ecosystem" where this is concerned. There are libraries that are built with a myriad of tools, one of them being Rollup, and there are also a ton of hand-crafted libraries. And yes, it will amplify the problem but only for Rollup itself.

But I do not really want to discuss this in more detail right now because I have been through a week of a ton of discussions around interop fueled by the question how to support package.exports in Node and it has been massively exhausting. To be honest, working on Rollup was giving me energy and inspiration but now it feels like depressing churn, even starting to ruin my family vacation in part. The thing is, I have the feeling nobody seems to respect and understand why these things like "auto" mode exist in the first place and I feel like I have to explain things over and make calls all the time that nobody likes anyway. At least it feels like there are always people who kindof pressure me into doing things one way or another instead of trying to understand the dilemmas I am facing. Maybe I should just ignore this discussion and go back to building actually cool features, the backlog is VERY long. Sorry I had to say this here, it is definitely not your fault, I just needed to vent some air.

[FredKSchott]

@lukastaegert I think you're doing a great job balancing a lot of different needs. This stuff is hard work, burnout is real! Make sure you're taking some time for yourself ❤️

[guybedford]

It could be useful to distinguish the cases of auto by treating the user entry points differently to the internal modules.

Entry points with auto = user wants to customize their own interface, so the tricks can be allowed fine.

Perhaps a new auto-local mode or similar could capture this distinction allowing the flexibility without affecting interop over non-entry point boundaries.

[Andarist]

@lukastaegert I highly appreciate your work, I follow it closely since eslint-plugin-tree-shaking, before you have started contributing to Rollup itself, much love for everything you do ❤️

My intention was never to pressure you to go in any direction - I know this stuff is really complicated and you have the best intentions for the ecosystem as a whole. There is just not a single good answer to this problem and it's OK to have different points of view. I was purely interested in more detailed reasoning behind the proposed change as it seemed (and you have confirmed it) that this would amplify the mentioned problem for Rollup users and this has seemed like a problem to me.

Please don't feel pressure to answer to those discussions that are happening immediately, take your free time with your family. Take it all at your own pace, even when not being on vacation. I know, and probably most of us here know it as well, it's often hard as we are deeply, often emotionally, invested in this stuff and we are very eager to discuss this at any time, often hurting our work-life balance. I feel that everyone here would totally understand waiting for a response longer.

[shellscape]

From the comments on this PR it seems that we're at a bit of an impasse on exactly how it should work. After having let this marinate for a week+, anyone have opinions/thoughts on whether we should move forward in this direction, pivot, or close the PR?

[danielgindi]

@shellscape I think that for the very least we should postpone this, as we seem to have other interoperability issues due to latest removal of an old feature.
We need to stabilize the interop while deciding on how it should actually work. Based on that- we can decide if this PR is good for us.

[danielgindi]

@shellscape @lukastaegert seems to me like this can be closed as interoperability was resolved at the root, right?

[lukastaegert]

Agree