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(ts): allow opt-in esModuleInterop (#1) #40

Closed
wants to merge 2 commits into from

Conversation

sambacha
Copy link

@sambacha sambacha commented May 8, 2022

Opt in compiler options

disable the following compiler options:

esModuleInterop: false
allowSyntheticDefaultImports: false

The two flags esModuleInterop and allowSyntheticDefaultImports enable interoperation between ES Modules and CommonJS, AMD, and UMD modules for emit from TypeScript and type resolution by TypeScript respectively.

Unfortunately these options are viral: enabling them in a package requires all downstream consumers to enable them as well . The TLDR is due to the way CommonJS and ES Modules interoperate with bundlers (Webpack, Parcel, etc.).

Solution

set both allowSyntheticDefaultImports and esModuleInterop to false.

Consumers can now opt into these semantics, it also does not require them to do so.
Consumers can always safely use alternative import syntaxes (including falling back to require() and import()), or can enable these flags and opt into this behavior themselves.

additional stuff

included tslib as its referenced in a compiler option, downlevelIteration. Updated typescript to 4.6.3, also updated the CI workflow.

@@ -72,7 +72,8 @@
"tslint": "^5.19.0",
"tslint-config-prettier": "^1.18.0",
"tslint-plugin-prettier": "^2.0.1",
"typescript": "4.5.4",
"tslib": "2.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't tslib be a dependency? as it's now imported by the compilation output, isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

Its used at build time to effect compiled output, I do not think it should be, its not like babel-runtime, can double check ofc

@alcuadrado
Copy link
Member

Thanks, @sambacha! I wasn't aware of the viral property of those settings.

I just left a small question re tslib

@sambacha
Copy link
Author

sambacha commented May 9, 2022

Thanks, @sambacha! I wasn't aware of the viral property of those settings.

I just left a small question re tslib

Heh I am quoting from here: https://www.semver-ts.org/#module-interop

There is an RFC from ember that this is based of off (semver-ts) that goes into more detail, im mobile otherwise i would link it

@alcuadrado
Copy link
Member

Great site! I'd always wanted something like this.

it is enough to note that changing from esModuleInterop: true to esModuleInterop: false on a package which emits is a breaking change

I think we should release a new major for this then.

@paulmillr what's your take on this?

@paulmillr
Copy link
Collaborator

Any disadvantages in these? What's the behavior of the changed options on our future addition of esm support?

@sambacha
Copy link
Author

Any disadvantages in these? What's the behavior of the changed options on our future addition of esm support?

If you are going to add ESM Support, might as well wait for that before doing this if as @alcuadrado suggested making a major release. FWIW I looked at some of the downstream users of this library and at first glance do not think any of the major users would be affected. Then again, YMMV.

It makes more sense IMHO to do this with an ESM supported package, in fact that was the motivating reason behind this originally.

@alcuadrado
Copy link
Member

For this package to be ESM compatible all of its dependencies have to be, right? @paulmillr mentioned that not all of them support it. Is this still the case?

@paulmillr
Copy link
Collaborator

Superseded by #41. I did not include tslib though

@paulmillr paulmillr closed this Jun 14, 2022
@sambacha
Copy link
Author

Superseded by #41. I did not include tslib though

Thanks @paulmillr - yea no need for it. Apologies for the delay in response.

Concerns

  • There might be an issue for distributing it over SRI hashes as ESM does not support the spec for it
  • There are small changes if your using rollup vs typescript in artifacts, namely rollup will not produce the triple slash "///` type indicator
  • if the module you’re importing defines its API using inherited properties, you need to use the default import form (import fs from "fs"), or disable esModuleInterop
  • Without export default you get a improved/useful intellisense in your IDE

Other Compiler Options Affecting the Build Result

  • extends
  • importsNotUsedAsValues
  • preserveValueImports
  • jsxFactory
  • jsxFragmentFactory

Reference

All of these claims are documented with citations and references in this repo, with some examples to validate the behavior that is described: https://github.com/sambacha/typescript-cjs-esm-tests

Getting this info was more involved than one would have hoped.

Thanks again for these libraries, much appreciated for yalls work!

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.

3 participants