preserveModules mess with import order in esm/system output with circular dependencies

[Amareis]

• Rollup Version: from 2.28.2 (maybe and prior) to latest (fom github master)
• Operating System (or Browser): Any ESM-compatible (node, chrome)
• Node Version (if applicable):any ESM-compatible
• Link to reproduction (IMPORTANT, read below): https://repl.it/repls/PlumpRadiantBetatest

Expected Behavior

After build module tree should working.

Actual Behavior

It don't works, because of wrong import order (tag.js run before data.js).

Additional: read this for rationale of such import structure. In our real project there is muuuuuuch more files instead of single tag.js, and in webpack (or simple rollup build, or in browser after simple babel transpilation) all is working good.
https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

[Amareis]

To fix this rollup shouldn't play with import chain simplifying, at least if imported file participates in some circular chains.

[lukastaegert]

I tried running both the code in src and the code in dist in Node 14. The only entry point that worked in the src folder without immediately giving me this (typical circular dependency) error

ReferenceError: Cannot access 'data' before initialization

was tag.js. Neither src/index.js nor src/main.js were valid entries in an actual runtime.

When running dist/tag.js, I got exactly the same output as when running it in src. Could you please go into more detail how to reproduce the bug, i.e. in what situation the generated code behaves differently from the source code?

Also note that "import chain simplification" should no change relevant execution order as additional side-effect imports are generated as needed. But I am open to counter-examples.

That is not to say that there are no issues with execution order in Rollup, but all known issues are based on the fact that Rollup sometimes creates too few chunks and will never manifest if you use preserveModules.

[lukastaegert]

Admittedly, TDZ errors are not tracked properly as side-effects...

[Amareis]

It's really strange, since on node 14 at my computer main is working as supposed. I'll dive into it later.

[lukastaegert]

Ok, I managed to fix your example and yes, there is an issue, but no, it is not about import chain simplification. Because all imports are still in place, Rollup just "fixes" the order. And by fixing I mean: Sorting them by their (correct!) execution order. But you are right, with circular dependencies this sorting is actually breaking our neck because index, even though it is the first in execution order, has other dependencies which will be executed before index if it is the first import in main. Now this requires some though...

[lukastaegert]

I actually managed to set up a patch that will fix or rather replace the sorting with something that should handle circular dependencies well: #3840