fix(commonjs): produce code which works when __esModule is already defined

[bhovhannes]

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:

Description

Opening this MR after short discussion in https://github.com/rollup/plugins/pull/1038/files#r1048162869.

I have an issue in upgrading from "@rollup/plugin-commonjs"@21.1.0 to "@rollup/plugin-commonjs"@latest. I tracked problem down to version "@rollup/plugin-commonjs"@22.0.0 and it looks like all changes from "@rollup/plugin-commonjs"@21.1.0 to "@rollup/plugin-commonjs"@22.0.0 are in #1038.

After looking into what was changed I noticed that the if (n.__esModule) return n; check in getAugmentedNamespace helper was removed.

Unfortunately my codebase is complex and I cannot disclose code of some npm packages we have. But at least one of them already defines __esModule property. When commonjs plugin decides to use getAugmentedNamespace helper on that module it creates a broken code, because when being run, code of getAugmentedNamespace helper attempts to define __esModule property. That property already exists and code throws an exception.

[bhovhannes]

@shellscape I am a bit confused. I ran tests locally and they passed. What do I need to make GitHub run tests for this PR?

[lukastaegert]

I agree with the change but it would be very useful to add one test case for why you introduced the change. Otherwise, there is nothing preventing anyone who does not know about the problems from reintroducing the change.
As I understand it was an issue with an external dependency? You could for instance extend the esm-externals-undefined test with a new external dependency that defines __esModule in the way that your problematic dependency does (there is a checked-in node_modules folder in test for these kinds of dependencies).

[bhovhannes]

I agree with the change but it would be very useful to add one test case for why you introduced the change. Otherwise, there is nothing preventing anyone who does not know about the problems from reintroducing the change. As I understand it was an issue with an external dependency? You could for instance extend the esm-externals-undefined test with a new external dependency that defines __esModule in the way that your problematic dependency does (there is a checked-in node_modules folder in test for these kinds of dependencies).

I did not find esm-externals-undefined test (I found only snapshot), but I added a test which executes generated bundle and asserts if it works properly. There are a lot of snapshot tests checking the presence of getAugmentedNamespace helper, so I think adding a test checking the resulting output makes sense.
I was not able to find specifically which npm package is causing the problem. I have many interconnected deps in my project and when I pick some of them, everything works, but all of them does not work together. I tried to do that today as well but without success.
However, the test I've added fails without if (n.__esModule) return n; line in getAugmentedNamespace helper and passes when I add that line back, which is good 🙂 .

[lukastaegert]

Ah I am sorry, it is in the test/fixtures/function folder. Each sub-folder here is its own test. You add a new test by adding a sub-folder.
The advantage of these tests is that they are more realistic (no "loader" plugin or similar), self-contained i.e. do not clutter the overly large test.js file and automatically run the code as well as do a snapshot.
They are the preferred way of testing. Also I am not sure your test covers your issue as the dependency would have needed to be external as I understand. Your example should already have worked without your changes.
Would you mind migrating your test to test/fixtures/function or as I suggested if it makes sense, extend esm-externals-undefined.
As for your dependency, just add something new to test/node_modules that has the exports.__esModule = true or similar in its entry file.

[bhovhannes]

Ah I am sorry, it is in the test/fixtures/function folder. Each sub-folder here is its own test. You add a new test by adding a sub-folder. The advantage of these tests is that they are more realistic (no "loader" plugin or similar), self-contained i.e. do not clutter the overly large test.js file and automatically run the code as well as do a snapshot. They are the preferred way of testing.

I'll try to write test in test/fixtures/function. Thanks for clarification!

Also I am not sure your test covers your issue as the dependency would have needed to be external as I understand.

I think it does cover. For what I've seen, the code throws an exception at a runtime when the getAugmentedNamespace helper is injected in the final bundle. And getAugmentedNamespace can be injected in both cases, when the module is external or when it is not. I can be wrong here though. While I understand what this helper does I still don't clearly understand how plugin decides if it should inject it or not.

Your example should already have worked without your changes.

No, that test fails without my changes. As I wrote, "the test I've added fails without if (n.__esModule) return n; line in getAugmentedNamespace helper and passes when I add that line back".
I think my test is the best thing I have now to reproduce the issue. And it is the only test which checks if the resulting bundle actually works. That's why I am inclined to leaving it rather than removing in favor of some sort of snapshot test.

[lukastaegert]

function tests are not simply snapshot tests, the do run the code first. Additionally, they get the Ava t variable injected as a global variable into the code so that you can write assertions inline in the running code by do things like t.is(...). They are superior in most aspects to the tests in test.js and I would actually like to get rid of most of the test.js test cases if time would permit because I find this file unmanageable.
About the test, you are right, I was wrong, the helper would be injected here.

[lukastaegert]

But it also means that this is exactly a situation where we would want the wrapper, because we now have an ES module namespace without a prototype. I very much hope that this is not what your dependency did, at least that is why I think this change is ok.

I was not able to find specifically which npm package is causing the problem

I would have assumed your dependency was not ESM but rather an external CommonJS module that does this:

exports.__esModule = true

which would have the same effect but is a more real-world use-case.

[bhovhannes]

I would have assumed your dependency was not ESM but rather an external CommonJS module that ...

I didn't managed to get getAugmentedNamespace helper added with such setup. Both plugin and rollup work amazing and concatenate referenced CommonJS module with the entry chunk without adding helper. I think I do something wrong, because I see getAugmentedNamespace added in some function tests in similar setups. But when I copy files from correspondent functions folder to an empty dir, throw a primitive rollup.config.mjs there and run rollup, I get a completely different output.

The only case where I managed to get getAugmentedNamespace helper added with __esModule set in CommonJS file was a case where both ESM and CommonJS modules were used at a same time (transformMixedEsModules: true). I added a test for that in functions.

Also, I moved my existing test to functions. While it is super unlikely that someone will declare an export named __esModule in ESM module, that test fails without if (n.__esModule) return n; line. As technically it is possible to have such weirdly named export I believe this test still has rights to live.

[lukastaegert]

Awesome, thank you very much for sticking with me!

[bhovhannes]

Thank you! Your comments helped a lot in both understanding code better and improving merge request.

One last thing. Do you have access to merge and publish a new version? If not, can you please tag someone to let this merged and published?

[lukastaegert]

In plugins, we would usually let the PR wait for some days so that others can add their comments before merging.

[shellscape]

@bhovhannes the CI failure was a workflow that validates the title of your PR. we use conventional commit format in this repo. I've fixed that for you and as soon as CI passes we should be able to merge.

[bhovhannes]

@bhovhannes the CI failure was a workflow that validates the title of your PR. we use conventional commit format in this repo. I've fixed that for you and as soon as CI passes we should be able to merge.

Thanks!