-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
feat: create official TypeScript base config @docusaurus/tsconfig #9050
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
Size Change: +64 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
"esModuleInterop": true, | ||
"jsx": "preserve", | ||
"lib": ["DOM"], | ||
"moduleResolution": "Node16", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are considering making it an error to specify moduleResolution: "node16"
without "module": "node16"
at microsoft/TypeScript#54567. "module": "node16"
implies "moduleResolution": "node16"
, but unfortunately not the reverse. Using one without the other matching has some very strange, unpredictable, and dangerous effects.
Many of the new errors that we detected would occur from this change are in tsconfigs that inherit from the old @tsconfig/docusaurus
package. Would you be able to add "module": "Node16"
to this template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The moduleResolution
should be bundler
, because this config is shared for our users, who exclusively write client code. We use "moduleResolution": "node16"
here so that package.json exports
can be resolved, but if we use "module": "node16"
, then because users never specify "type": "module"
in their package.json (this actually breaks webpack, for some reason yet to be discovered), they will not be able to import ESM dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, can you change to module: esnext
with moduleResolution: bundler
, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I don't think anyone has played with the tsconfig since the last time I added node16
. I'm a bit caught up with MDN work recently so I may not be able to come back to it, but I (or @slorber) will surely play with it when we have time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you version @docusaurus/tsconfig? Would this have to wait until a major version bump of other docusaurus packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@docusaurus/tsconfig is already created for the next major, so before its release, we are free to do anything with it :) Do you anticipate the switch to moduleResolution: bundler
to be a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it could introduce new TS errors. With node16
and no "type": "module"
, resolution of imports of external libraries uses the types
, node
, and require
conditions when lookup up conditional exports, because the assumption is that these imports will be emitted as require
calls (this is part of why mixing --moduleResolution node16
with --module esnext
breaks some core assumptions in the compiler). With bundler
, the conditions are types
and import
. This better matches what Webpack is actually going to do during its own resolution, but we do occasionally see external libraries that have messed up the types for their import
entrypoint.
So, it’s really like a bugfix, but it could trigger an external library’s typing bug in a way that the current config doesn’t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It should be okay for us. Our versioning scheme typically allows "more type errors that are undeniably user bugs" across minor versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our CI started failing after 5.2 GA release, so it's a good reminder for me to handle this 😅
This will be fixed in #9258
Thanks
Breaking change
The old external
@tsconfig/docusaurus
package is now deprecated and you should use our new official versioned package@docusaurus/tsconfig
.The standard Docusaurus website TypeScript config becomes:
Motivation
Follow-up of the React 18 upgrade (#8961) including the usage of the automatic JSX runtime: we should now use
jsx: "preserve"
in our base default TS configAs discussed in the past: our base TS config should evolve alongside the codebase and be versioned (cf tsconfig/bases#122)
Test Plan
website + dogfood + CI
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
Automatic JSX runtime needs use to change the config to
jsx: "preserve"
instead ofjsx: "react"
.See also: