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

docs: replace deprecated moduleResolution #3018

Merged
merged 5 commits into from
Jul 21, 2024

Conversation

MarlonPassos-git
Copy link
Contributor

Currently, in the recommendation documentation for tsconfig files, when setting

//...
"moduleResolution": "Node",
//.

a message appears indicating that this value is deprecated and recommends using Node10.

image

This pull request updates the documentation to recommend using the suggested setting.

@MarlonPassos-git MarlonPassos-git requested a review from a team as a code owner July 15, 2024 05:41
Copy link

netlify bot commented Jul 15, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit e4ada17
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/669d6e21a6940700081825a6
😎 Deploy Preview https://deploy-preview-3018.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Shinigami92
Copy link
Member

Node10 also sounds like a very outdated value to me 🤔
IMO this should be replaced by a value that does not deprecate in a while next time again.

I personally use "Bundler" today, but this is due to I only use tsup, esbuild or Vite for my projects

@MarlonPassos-git
Copy link
Contributor Author

@Shinigami92 I usually use NodeNext or node16. What are your thoughts on including something like this in the documentation:

"moduleResolution": "Node16", // "NodeNext" or "Bundler"

This way, users will have a range of compatible and non-deprecated options.

@Shinigami92
Copy link
Member

@Shinigami92 I usually use NodeNext or node16. What are your thoughts on including something like this in the documentation:

"moduleResolution": "Node16", // "NodeNext" or "Bundler"

This way, users will have a range of compatible and non-deprecated options.

Does not sound wrong to me 👍

@faker-js/maintainers what's your thoughts about that?

@Shinigami92 Shinigami92 added the s: needs decision Needs team/maintainer decision label Jul 15, 2024
@Shinigami92 Shinigami92 added this to the vAnytime milestone Jul 15, 2024
@Shinigami92 Shinigami92 added the c: docs Improvements or additions to documentation label Jul 15, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Jul 15, 2024

We require at least node 18 so Node16 should be a safe value to use.
We could go for a modern value as well too.

@ST-DDT ST-DDT added the p: 1-normal Nothing urgent label Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (8a35e70) to head (8d57361).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3018      +/-   ##
==========================================
- Coverage   99.97%   99.96%   -0.01%     
==========================================
  Files        2747     2747              
  Lines      226148   226148              
  Branches      591      591              
==========================================
- Hits       226093   226074      -19     
- Misses         55       74      +19     

see 2 files with indirect coverage changes

@Shinigami92
Copy link
Member

We require at least node 18 so Node16 should be a safe value to use. We could go for a modern value as well too.

I think this is exactly the problem and not how that property works. It is totally confusing for persons that are not read into the details and just assume that the number after that means that.

Node10, Node16 and NodeNext are telling the TS compiler how imports should work. So e.g. more like CJS or ESM and if require, export = and import ('file.js') (.js suffix) are read and interpreted.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 15, 2024

All values we put there have side effects (maybe except node10 and less so for bundler).
Which values cause issues depends on the build tools the user is using.
e.g. Node16 causes issues for tsc, but neither for vue-tsc or vite.
Also there are limitations regarding the module flag as well.

"moduleResolution": "Node10", // "Node16", "NodeNext" or "Bundler"

That's why I consider using the more modern suggestion bundler (with the other values as comments).

"moduleResolution": "Bundler", // "Node10", Node16" or "NodeNext"

I couldn't find any limitations regarding that option except for node16 and ts5 (matching our requirements), but I found hints at other incompatibilities such as tooling and backwards compatibility, but I was unable to fully grasp them.

EDIT: If we are going for the least likely values to break, then those are only node10 and bundler and not NodeNext.
EDIT2: We can likely remove the esInterop property as well

@MarlonPassos-git
Copy link
Contributor Author

MarlonPassos-git commented Jul 15, 2024

I like this:

"moduleResolution": "Bundler", // "Node10", Node16" or "NodeNext"

EDIT: Now about removing esInterop, you're in charge. I can update the pr with this modification too

Shinigami92
Shinigami92 previously approved these changes Jul 15, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Jul 15, 2024

You might have to change the code highlighting to jsonc or something.

@MarlonPassos-git
Copy link
Contributor Author

MarlonPassos-git commented Jul 15, 2024

adjusted

@ST-DDT ST-DDT requested review from a team and Shinigami92 July 15, 2024 22:35
@ST-DDT ST-DDT requested a review from a team July 16, 2024 11:33
@ST-DDT ST-DDT changed the title docs: replace deprecad moduleResolution docs: replace deprecated moduleResolution Jul 21, 2024
@ST-DDT ST-DDT removed the s: needs decision Needs team/maintainer decision label Jul 21, 2024
<