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

fix: default interop compat for "module" condition #179

Merged
merged 2 commits into from May 1, 2023

Conversation

guybedford
Copy link
Contributor

This adds default import interop compat when resolving using the "module" condition.

Many users are still expecting import tslib from 'tslib' to work, since this is supported for the CommonJS interface in both Node.js and browsers, but if building with the "module" condition outside of Webpack (eg as import map generators like JSPM do), this results in breaking the tslib interop.

It is always recommended for all exports conditions to provide the same interface. This decoration effectively provides the identical interfaces between the ESM and CJS forms of tslib, maximizing interop.

//cc @weswigham

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn’t we do export default tslib in modules/index.js as well then?

@guybedford
Copy link
Contributor Author

Yes, exactly.

@Andarist
Copy link
Contributor

Andarist commented May 1, 2023

Many users are still expecting import tslib from 'tslib' to work

This package ships an import condition though so the default-related interop shouldn't be broken here in the first place (although perhaps it was with some prior versions of this package).

@guybedford
Copy link
Contributor Author

The problem I had here is JSPM supports the "module" condition because it can support CJS loading ESM, but it uses Node.js-style interop for the "module" condition which then breaks down. In general JSPM will be moving away from the "module" condition now for this reason...

@Andarist
Copy link
Contributor

Andarist commented May 1, 2023

My understanding of the module condition was always that the files referenced by it should be authored in ESM but still consumable by both require and import loaders since the tool through which it is consumed is capable of that (like a bundler). Based on that, I'm somewhat confused as to what kind of an interop is needed in this situation 🤔

@guybedford
Copy link
Contributor Author

@Andarist this is because bundlers in implementing the module condition break Node.js-style interop by default since they use "bundler ESM interop" which is different. Remember import x from 'cjs' is always export default module.exports!

@guybedford
Copy link
Contributor Author

All this to say, the default + NS pattern is a kind of approach that can work for best interop. Node.js does it with its core modules too.

@Andarist
Copy link
Contributor

Andarist commented May 1, 2023

Remember import x from 'cjs' is always export default module.exports!

yeah, i know how this one goes but that's only when the 'cjs' package doesn't have an import condition. This one has that so I'm not exactly sure what kind of scenario this PR is fixing.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have feelings about this that mostly involve lamenting how the ecosystem has gotten to the point where redundant API shapes like this seem required. I don't think more packages bending over backwards to help out tools that are mis-processing files or confused users will ultimately help the ecosystem - after all, the more packages that do this, the more people expect it, the more it's an issue - it just helps us triage less confusion. :(

@andrewbranch andrewbranch merged commit 4e79d03 into microsoft:main May 1, 2023
6 checks passed
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 this pull request may close these issues.

None yet

5 participants