fix(commonjs): do not create fake named exports

[lukastaegert]

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

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:
#423

Description

This one came up via #423 where a lot of unused assignments were added to the code. As it turns out, this plugin was already running its own version of "synthetic named exports", which unfortunately was providing sub-optimal results that did not work well with circular references and also left some non-treeshakeable residue.

This fixes this by only rendering named exports when we do not have to pick them off module.exports. In that situation, we rather leave this to Rollup's syntheticNamedExports handling.

Also, this adds a /*@__PURE__*/ annotation to unwrapExports because otherwise some unused default exports were leaving some calls in the code that were again not treeshakeable.

[shellscape]

This is tough. It's a breaking change for anyone relying on the long-running bad behavior. It's a fix to anyone impacted negatively (issue reporter). Not sure which way to lean on this.

[shellscape]

@lukastaegert since namedExports was deprecated I think we'll go with a safe major. #410 is related.

[lukastaegert]

Not sure. The plugin will still create named exports, just not in some edge case scenarios. I think you would only notice this if directly bundling an affected file as an entry point. For everyone else, this is a bug fix. But I guess you are right.

[danielgindi]

I think that this makes a much healthier interop. I've seen weird unexpected exports in tests, even in those subtle cases of named exports vs default export, cjs vs es. They are gone now after this is merged.

[lukastaegert]

It is kind of fixing v12 to how it was supposed to work.

[lierdakil]

As far as I can tell, this breaks CommonJS modules that have both default and named exports, see #451, #454, #465.

[FredKSchott]

If a common.js module has named exports (and __esModule === true) isn't the "default" named export always meant to be equivalent to the ESM default export?

[lierdakil]

Disclaimer: I'm sleep deprived because of spending the time I should've been sleeping on debugging. So at this point I might be missing something. But here's more or less what I've gathered from staring at diffs and changelogs. Feel free to correct me.

I'm not all that familiar with CJS-ESM interop standards, but at least Babel converts ESM to CJS along the lines of what @FredKSchott describes.

However, here it is a bit more complicated. Commonjs-plugin tries to convert CJS to ESM, which can be handled by rollup. Prior to this PR, export default was emitted together with a separate export statement for each of the named exports. Which ultimately resulted in some unused code (potentially a lot). This PR removes separate export statements for named exports, so that only export default is emitted, which exports the whole module if there is no exports.default (this is what unwrapExports does). This works fine if you have only named exports because of a fallback mechanism (basically the module is treated as the old es5 cjs, i.e. __esModule is essentially ignored). When you have both default and named exports, however, the named exports get lost, because exports.default takes precedence for export default.

Sorry if my explanation is overly convoluted.