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

update config settings and readme #2

Merged
merged 2 commits into from
Sep 18, 2023
Merged

update config settings and readme #2

merged 2 commits into from
Sep 18, 2023

Conversation

fox1t
Copy link
Member

@fox1t fox1t commented Sep 16, 2023

This is a breaking change.

  • version bump to 2
  • module and moduleResolution are set to NodeNext, which follows the Node.js ESM vs. CJS resolution (exports and main fields)
  • adds more examples to readme based on the use case

Checklist

bump version to 2.0.0
@fox1t
Copy link
Member Author

fox1t commented Sep 16, 2023

@fastify/typescript FYI

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'm not convinced by doing this change right now. It means that our typescript modules would become ESM.

As far as production system goes, ESM is almost ready, as ESM loaders will become stable soon.

Out of prudence I would keep it commonjs still.

The other reason why I'm on the fende is that it does not make much sense to produce ESM with TS and stick all other modules as commonjs.

@fox1t
Copy link
Member Author

fox1t commented Sep 16, 2023

I'm not convinced by doing this change right now. It means that our typescript modules would become ESM.
This is not true. Let me explain how NodeNext works.

NodeNext says TypeScript to follow Node.js rules around how it resolves ESM and CJS files. In practice, this means that TypeScript will read the nearest package.json file in the scope and search for the "type" field or the absence of it. The rule, therefore, is simple.

If type is absent or "type": "commonjs" the emitted code will be CommonJS, with .js extension. Moreover, tsc will complain if you use ESM-only properties/features in your files. If you really want to emit .mjs files, use .mtsextension.
On the other hand, if "type": "module" is set, the sources will be compiled to the ESM with the .js extension. If you want to emit .cjs files, use the .cts extension.

The "following the Node.js rules" goes also for the exports field. If the type is set, regardless of the value, TypeScript will check the exports field to know where the compiled code and the types are located. If the type field is not set, it will check for the old main and types fields.

TypeScript should have been doing this since the beginning, but they needed to wait on Node.js to implement ESM.

That said, this is a breaking change because we are increasing the minimum Node.js version from version 10 to version 16 since we are changing the target of the compilation to ES2022 from ES2018.

I hope this helps you understand how it works and why it is crucial to use this retro-compatible configuration.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you add the above in the README? Thx!

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@fox1t
Copy link
Member Author

fox1t commented Sep 17, 2023

Could you add the above in the README? Thx!

Done!

@fox1t fox1t requested a review from mcollina September 17, 2023 13:42
Copy link

@rozzilla rozzilla left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@fox1t fox1t merged commit f8bf1f8 into master Sep 18, 2023
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.

5 participants