[commonjs] cross import between entry commonjs modules is broken

[csr632]

• Rollup Plugin Name: @rollup/plugin-commonjs
• Rollup Plugin Version: Latest
• Rollup Version: Latest
• Operating System (or Browser): MacOS, Repl.it
• Node Version: v12.16.3

How Do We Reproduce?

https://repl.it/@csr632/rollup-plugin-repro

Expected Behavior

checkout dist/test1.js , it access test2 module as test2.__moduleExports, which is clearly undefined.

Actual Behavior

dist/test1.js should log test2 correctly

Output format is esm, so maybe related to #400. But in my case, I have multi entry modules.

[danielgindi]

Not the same as #400.

@lukastaegert Can you please explain the role of export { moduleName as __moduleExports };? I can't find any justification for it in the code, as it's a duplicate of export default moduleName.
What happens multi-entry rollups, the condition which exports it - fails. But when importing - it always imports __moduleExports from cjs (unless there's dynamic requires detected).
Seems to me like if we remove the __moduleExports, and just use the default import, everything that worked before will still work, only with less garbage.

[danielgindi]

@lukastaegert Okay I think I got it. Trying to find a way to make it play well with multiple entries.

[lukastaegert]

That’s good because honestly I do not know, this was apparently from before I started on this project 🙄

[danielgindi]

@lukastaegert Ok so just in case I forget later - it's basically a shortcut to the stuff that's stuffed onto module.exports.foo = bar (vs. module.exports = ...), for other internal modules (in the same rollup) to use, to avoid the unwrapExports(...) from pulling a member named default in case it exists. It basically allows to workaround all the babel __esModule/default hacks internally, so it refers to the right objects.

Also most of its references gets shaken off by rollup and replaced with default import when relevant and possible. That's why it's hard to see the source of the problem in the REPL.

My other PR from today actually has a test that fails when I remove the whole __moduleExports thing, and that is what clarified things to me. Pure luck.

[csr632]

@danielgindi
The issue still exists in latest master branch.
This is reproduction:
https://github.com/yyx990803/rollup-commonjs-bug
Build the latest master branch and yarn link it to the repo, the issue still exist.

[danielgindi]

@csr632 Could you add a test in the commonjs package? I think we can't use the git master in the REPL, as it's not compiled.

[csr632]

@danielgindi
You can build and test any branch locally by yarn link(or npm link):

cd /rollup-plugins/packages/commonjs
yarn build
yarn link

cd /path/to/reproduce/repo
yarn link @rollup/plugin-commonjs
yarn build

I am currently working on other stuff, I can add the test later.

[danielgindi]

@csr632 I want to see a REPL with the problem reproduced with latest master

[csr632]

@danielgindi
They have published the master branch. Here is the reproduction with latest plugins: https://repl.it/@csr632/rollup-plugin-repro

Checkout the dist/test1.js. The __moduleExports still exists.

[danielgindi]

@lukastaegert For some reason, when debugging this earlier (probably a different rollup version) while working on the PR, the moduleInfo.importers had information about test2.js being imported by test1.js, now the importers is empty.
Do you have any idea why?
In order for this to work in multi-entry bundles, we need to know that the current entry is being required by one of the modules (and thus not acting only as an entry).

[lukastaegert]

importers is filled dynamically and may be incomplete while the graph is being built. I fear this is what hit you and thinking about this, I'm not sure how to solve this: I mean if you are very unlucky, it might be the very last module that is added to the graph that imports the entry point.

[lukastaegert]

So I see only two solutions

1. The export is added to ALL entry points
2. We do not user __moduleExports when importing from entry points but work around this, possibly with some interop code. This might be tricky because I keep forgetting what the reason for having this export was in the first place, I know you wrote it somewhere

[danielgindi]

So I see only two solutions

1. The export is added to ALL entry points

2. We do not user __moduleExports when importing from entry points but work around this, possibly with some interop code. This might be tricky because I keep forgetting what the reason for having this export was in the first place, I know you wrote it somewhere

If I recall correctly- this was to avoid conflicts where a member named default is exported from CJS, and unwrapExports(...) will unwrap that.

We can always guess, but we will miss.
Adding _moduleExports to all entry points when output format is CJS will work of course, may annoy some people, but that’s what we got. We may be able to detect whether we need to add the __moduleExports, but I’m not sure if it’s conditional when importing it back.

[lukastaegert]

Then maybe go for the simple solution now, and we can think if we can do better when people are actually annoyed.

[danielgindi]

@lukastaegert Can we at least detect inside the plugin that it has multiple entry points? This would allow reducing the cases where it will pollute the output

[lukastaegert]

Not reliably. Even if there is only a single entry, plugins can theoretically emit additional entries at any time until building the graph is complete. E.g. the transform hook of a module could emit a new chunk for a worker that is created in the module.

[danielgindi]

Well, I tried.
The end result is that the output (when format: 'cjs') will end up like this:

exports.__moduleExports = 'BAZBAZ';
exports.default = 'BAZBAZ';

Yeah we said it will pollute. But the problem is that this is actually instead of:

module.exports = 'BAZBAZ';

So it always results in the following warning:

Entry module "fixtures/function/exports/main.js" is using named and default exports together. Consumers of your bundle will have to use `chunk["default"]` to access the default export, which may not be what you want. Use `output.exports: "named"` to disable this warning

Which is a very important warning.

I'm starting to think there's no good solution for this.

[lukastaegert]

I started #481 to move all interop issues including this one forward.

[lukastaegert]

I can confirm this is resolved by #501.