Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why doesn't rollup (and other tooling) insert the node16 compatibility layer during ESM->CJS transpilation? #109

Closed
MilanKovacic opened this issue Oct 19, 2023 · 7 comments · Fixed by #110

Comments

@MilanKovacic
Copy link

I encountered a node16 (cjs) resolution mode problem after the transpilation of ESM module to CJS with rollup, and had to resort to inserting the compatibility layer in the code itself:

if (typeof module !== "undefined" && module.exports) {
  module.exports = defaultExport;
  module.exports.default = defaultExport;
}

I am curious as to why some tools don't insert the compatibility layer, since they already set the exports.default and exports.__esModule?
I was thinking about building a rollup plugin to insert it, but would like to know if there are any caveats/downsides.

@andrewbranch
Copy link
Collaborator

andrewbranch commented Oct 19, 2023

Note that I think I’m going to silence this error in node16 (cjs); see #108. It’s currently being issued because the problem is a feature of the JS source code alone, so arguably any resolution algorithm that resolves to this JS sees the problem, but in reality, the only importers that process this in an unexpected way are

  • Node.js ESM files
  • esbuild, in .js/.ts with "type": "module", or in .mjs/.mts
  • Webpack, in .js with "type": "module", or in .mjs

So I might leave it as a problem for bundler 🤔. (But on the other hand, the bundler analysis doesn’t currently follow the module condition, and it probably should, to more accurately model how most/all bundlers work.) It’s really hard to figure out where to present some of these problems.

I don’t know the answer to your question. The compatibility layer does change the semantics of the code in a way that is observably different from real ESM imports. The docs page mentions that you’ll be able to do importedThing.default.default.default.default... and maybe that could cause a problem if someone is, say, iterating over the keys of the object, or trying to serialize the thing they imported.

@andrewbranch
Copy link
Collaborator

I think my practical advice would be

  • If you’re building a dual package, don’t worry about this. arethetypeswrong will probably update to not show anything for properly configured dual packages.
  • If you’re building CJS only, maybe just don’t use export default in the first place.

@MilanKovacic
Copy link
Author

MilanKovacic commented Oct 20, 2023

Thank you for the information.
I am working on a dual package (type: module).
Tracing resolutions for node16 (cjs), typescript locates the .d.cts file containing this code:

export = defaultExport;

declare namespace defaultExport {
  export {
    // Other exports
  };
}

...along with the .cjs file containing both exports.default, and exports.__esModule (transpiled by rollup).
From my understanding — without the compatibility layer — esbuild and webpack would synthesize the default export resulting in unwanted behavior (actual default export being in .default). Am I correct?

It seems like removing the default export is the easiest option, but I am curious to know exactly how it works :)

@andrewbranch
Copy link
Collaborator

esbuild and Webpack both go out of their way to act more like Node.js when they see Node ESM cues like .mjs and package.json "type": "module". (Webpack only does this for JS files, not TS files. esbuild does it for both.) I have a bit of info about it here: https://andrewbranch.github.io/interop-test/#synthesizing-default-exports-for-cjs-modules

But I think that won’t affect you here, because Webpack and esbuild, like ESM files in Node.js, should resolve to the ESM entrypoint of your package. I think there’s no problem. I definitely plan to make some updates soon to arethetypeswrong to do a better job of analyzing and calling out exactly what the potential problem is here.

Is the project you’re working on available as a test case, by any chance?

@MilanKovacic
Copy link
Author

Interesting. The only problematic tool I can think of is Jest, as ESM support just entered experimental stage, but I am not familiar in exactly how it's resolution works (but did encounter .default problem in various packages).

The project I am working on is single-spa-react, specifically, this branch/PR is what I'm working on: single-spa/single-spa-react#197 . Feel free to use it.

I was thinking of setting up, and open-sourcing a comprehensive integration test suite with common tooling, and resolutions. What do you think?

@andrewbranch
Copy link
Collaborator

Sounds great!

@andrewbranch
Copy link
Collaborator

My existing fixtures already had some coverage for the changes I made, so I decided not to try to turn your PR into a test. But you may find with the latest version of @arethetypeswrong/cli, you don’t need to add the manual compat code. Let me know how it works out 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants