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.

Sign up for GitHub

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 TypeScript to v5.2.2 #3315

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Update TypeScript to v5.2.2 #3315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2014838
Update TS to v5.2.2
obulat Nov 5, 2023
6ef915b
Update vue-tsc
obulat Nov 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ repos:
additional_dependencies:
- [email protected]
- [email protected]
- typescript@4.9.5
- typescript@5.2.2

- repo: https://github.com/koalaman/shellcheck-precommit
rev: v0.9.0
Expand Down
4 changes: 2 additions & 2 deletions frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,14 @@
"tailwindcss": "^3.3.5",
"talkback": "^3.0.1",
"ts-node": "^10.9.1",
"typescript": "^4.9.5",
"typescript": "^5.2.2",
"vue-demi": "^0.14.6",
"vue-i18n-extract": "^2.0.7",
"vue-jest": "^3.0.7",
"vue-loader": "^15.10.0",
"vue-server-renderer": "^2.7.14",
"vue-template-compiler": "^2.7.14",
"vue-tsc": "1.2.0",
"vue-tsc": "1.8.22",
"webpack": "^4.46.0"
},
"browserslist": [
Expand Down
1 change: 0 additions & 1 deletion frontend/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"@pinia/nuxt",
"@sentry/core"
],
"typeRoots": ["./typings", "./node_modules/@types"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

So I checked locally and the typings are definitely still working!

@obulat It might also be necessary/wise to update vue-tsc, I think 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🤷

Copy link
Collaborator

@sarayourfriend sarayourfriend Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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!

"paths": {
"~/*": ["./src/*"],
"~~/*": ["./*"]
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"@openverse/eslint-plugin": "workspace:0.0.0",
"prettier": "2.8.3",
"prettier-plugin-tailwindcss": "0.4.1",
"typescript": "4.9.5",
"typescript": "5.2.2",
"vue-tsc": "1.2.0"
},
"pnpm": {
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"eslint-plugin-unicorn": "^48.0.1",
"eslint-plugin-vue": "^9.17.0",
"eslint-plugin-vuejs-accessibility": "^2.2.0",
"typescript": "^4.9.5",
"typescript": "^5.2.2",
"vue-eslint-parser": "^9.3.1"
},
"devDependencies": {
Expand Down
Loading