Update TypeScript to v5.2.2 #3315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Olga Bulat <[email protected]>
obulat
requested review from
fcoveram,
sarayourfriend and
dhruvkb
and removed request for
fcoveram
openverse-bot
added
the
🚦 status: awaiting triage
obulat
added
💻 aspect: code
|
Size Change: 0 B Total Size: 946 kB ℹ️ View Unchanged
|
obulat
added
🟨 priority: medium
| @@ -15,7 +15,6 @@ | |||
| "@pinia/nuxt", | |||
| "@sentry/core" | |||
| ], | |||
| "typeRoots": ["./typings", "./node_modules/@types"], | |||
There was a problem hiding this comment.
Removing the reference to typings would (theoretically) exclude our custom type definitions in that directory from being used.
I actually noticed a similar thing locally, but couldn't reproduce it when running the type check, and was able to fix it by adding ../node_modules/@types (the monorepo root) as well:
Suggested change
| "typeRoots": ["./typings", "./node_modules/@types"], | |
| "typeRoots": ["./typings", "./node_modules/@types", "../node_modules/@types"], |
However, if our custom extensions of library types are working, then there might not be any need for typings at all 🤷
There was a problem hiding this comment.
So I checked locally and the typings are definitely still working!
@obulat It might also be necessary/wise to update vue-tsc, I think 🤔
There was a problem hiding this comment.
However, if our custom extensions of library types are working, then there might not be any need for typings at all
I think adding the typeRoots helps locating the typings instead of making TypeScript look through all of the possible sources in the whole monorepo. However, I couldn't find the correct path to make it work, and when I take it out, it works 🤷
There was a problem hiding this comment.
I see! In that case, this appears to work!
"typeRoots": ["./typings", "../node_modules"],
If I remove the node modules entry or add /@types I get the same errors you mentioned. Using this list, however, works exactly like leaving the option out altogether.
No idea whether it's preferred to have the option explicit though. Does it save much time when type checking, I wonder?
A non-scientific test, the first run is without the typeRoots set explicitly, and the second is with it set:
➜ openverse git:(update/ts) ✗ time just p frontend types
pnpm --filter frontend run types
> @openverse/[email protected] types /home/sara/projects/openverse/frontend
> vue-tsc -p .
just p frontend types 10.47s user 0.47s system 202% cpu 5.392 total
➜ openverse git:(update/ts) ✗ time just p frontend types
pnpm --filter frontend run types
> @openverse/[email protected] types /home/sara/projects/openverse/frontend
> vue-tsc -p .
just p frontend types 10.62s user 0.46s system 205% cpu 5.393 total
One run of each isn't enough to draw conclusions but the difference is so narrow that even if it mattered either way I wouldn't care 😅
Leaving it out means less configuration which is good in my book!
Signed-off-by: Olga Bulat <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
No issue for this update
Description
This PR updates the TypeScript version to the latest stable, v5.2.2.
I removed
typeRootssetting fromtsconfigbecause with it, type checks were failing withThis happens if types are specified in
typesparameter in tsconfig, but not included in thetypeRoots1.@sarayourfriend, I'm not sure what effect excluding
typeRootswill have.I found a very old note about this setting being added for backward compatibility with typings: microsoft/TypeScript#22217 (comment)
And here are the docs about the change that caused the new behavior during resolution, in TS 5.1: https://devblogs.microsoft.com/typescript/announcing-typescript-5-1-rc/#typeroots-are-consulted-in-module-resolution
Testing Instructions
The CI should pass, both VS Code and WebStorm should not have additional warnings about TS or unresolved files in JS.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin
Footnotes
Related test file in TS repository ↩