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(types): Fix module/moduleResolution options for TypeScript 5.2 #3251

Merged
merged 2 commits into from
Jul 16, 2023

Conversation

andrewbranch
Copy link
Contributor

👋 Hey, I work on the TypeScript team, and in 5.2 we’re planning to break your tsconfig, so I’m opening a PR to preemptively fix it. Specifically, we’re going to enforce that module and moduleResolution have to be set to the same value when either is node16 or nodenext.

You’re currently using moduleResolution: node16 in combination with both module: commonjs and module: esnext in order to achieve dual module format emit with tsc. tsc isn’t really designed to work with those combinations, as explained in the afore-linked PR. Those limitations are why you were susceptible to issues like #2759 and need the eslint rule that enforces file extensions on import paths. Really, TypeScript should be doing that validation for you. (By the way, it looks like #2759 was never released, as the current release still has type resolution errors.)

This PR makes a slight modification to your build scripts to ensure that tsc sees an accurate value for package.json "type" when compiling, which (with the right tsconfig settings) affects its type checking, module resolution, and emit to all be in sync. You might also be interested in tracking microsoft/TypeScript#54593 which, if we ever resolve, aims to make this kind of thing easier.

Feel free to use this PR, edit it, or close it and do something different. Just wanted to give you a heads up and hopefully get you started down the right track.

@@ -29,6 +29,7 @@
"main": "lib/index.js",
"types": "lib/index.d.ts",
"module": "lib/esm/index.js",
"type": "commonjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn’t actually do anything as is, but having a placeholder makes using sed a lot easier.

Comment on lines -26 to -28
"esModuleInterop": true,
"moduleResolution": "node16",
"resolveJsonModule": true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

--module node16 implies all three of these things (though I had to add the explicit esModuleInterop back; see the comment above)

@fb55 fb55 merged commit 71a7932 into cheeriojs:main Jul 16, 2023
@fb55
Copy link
Member

fb55 commented Jul 16, 2023

@andrewbranch Thank you so much for the helping hand!

@andrewbranch andrewbranch deleted the fix-module-resolution branch July 24, 2023 17:46
@unional
Copy link

unional commented Sep 16, 2023

Hi, coming here from Twitter discussion: https://twitter.com/atcb/status/1694038339312083055

I noted 3 things in that thread:

  • "esModuleInterop: true" does not work with Node16/NodeNext
  • "module: commonjs" defaults moduleResolution to classic, not Node
  • the CJS build will fail if the package depends on libraries doing Node16 (i.e. with .js)

For the first one, you can see this report:
https://github.com/cyberuni/typescript-module-resolutions-demo/blob/main/tests/node16-es/test-result.5.0.0-dev.20230103.md

Notice that there are errors in the commonjs section in the table, and the error varies depends on how the dependence are written.

For the second one, it is relative minor as most of the time we will also define the moduleResolution to begin with, but it was my understand that the moduleResolution is default to classic by default in that case. It could have changed in newer TypeScript versions.

For the third one, it is the case related to ESM package using exports field and missing the main field. When running tsc under CommonJS, those packages will fail to load.

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

Successfully merging this pull request may close these issues.

3 participants