Update to Typescript 5.1 #52621
Merged
Update to Typescript 5.1 #52621
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
noahtallen
force-pushed
the
update/typescript-5
branch
from
98b3ff1
to
b2a3a63
Compare
|
Size Change: 0 B Total Size: 1.44 MB ℹ️ View Unchanged
|
9 tasks
|
Flaky tests detected in af9ca66. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5559123464
|
noahtallen
force-pushed
the
update/typescript-5
branch
from
d3c85d0
to
7e517a1
Compare
noahtallen
commented
Jul 14, 2023
noahtallen
force-pushed
the
update/typescript-5
branch
from
7e517a1
to
28984ca
Compare
noahtallen
commented
Jul 14, 2023
| @@ -60,6 +60,8 @@ function createRunHook( hooks, storeKey, returnFirstArg = false ) { | |||
| if ( returnFirstArg ) { | |||
| return args[ 0 ]; | |||
| } | |||
|
|
|||
| return undefined; | |||
There was a problem hiding this comment.
TS was complaining about this code path not returning anything, so I just added return undefined to satisfy it. Interestingly, TS is supposed to be better at handling this scenario in TS 5.1, but I suppose that improvement might not apply if the types are in JSDoc.
noahtallen
commented
Jul 14, 2023
| @@ -8,9 +8,7 @@ | |||
| "gutenberg-test-env", | |||
| "dom-scroll-into-view", | |||
| "jest", | |||
| "@testing-library/jest-dom", | |||
| "snapshot-diff", | |||
| "@wordpress/jest-console" | |||
There was a problem hiding this comment.
tsc would not succeed with these references. Doesn't seem like they're needed, as jest-console is an internal package, and snapshot-diff bundles its own types.
noahtallen
commented
Jul 14, 2023
| 'error', | ||
| { | ||
| prefer: 'type-imports', | ||
| disallowTypeAnnotations: false, |
There was a problem hiding this comment.
We frequently import type annotations via JSDoc in Gutenberg, so we need to keep this setting off :)
noahtallen
force-pushed
the
update/typescript-5
branch
2 times, most recently
from
537651d
to
ca34e2a
Compare
noahtallen
requested review from
kevin940726,
adamsilverstein,
ajitbohra,
gziolo,
ntwb and
nerrad
as code owners
noahtallen
added
[Type] Build Tooling
|
Edit: left this comment in the wrong repo 😆 |
noahtallen
force-pushed
the
update/typescript-5
branch
from
af9ca66
to
4d8f95e
Compare
gziolo
reviewed
Aug 7, 2023
packages/eslint-plugin/CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Enahncement | |||
There was a problem hiding this comment.
Out of curiosity, does TypeScript 5 has some breaking changes: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#breaking-changes-and-deprecations. Do you think it might impact ESLint? Maybe we should follow-up with a breaking change to stay on the safe side? It would also require doing a major version bump for @wordpress/scripts.
There was a problem hiding this comment.
I don't think we need a major update -- I only updated the typescript eslint plugins to the latest minor update (though they do have major updates available). This means the eslint plugins should still support everything they did before, but also have typescript 5 support now. So this shouldn't require people to change anything -- typescript is only a peer dependency of eslint-plugin after all
There was a problem hiding this comment.
Thank you for the clarification. Works for me 👍🏻
|
Everything looks good from what I can see from a quick check on this PR. That's excellent news! It becomes easier to bring TS to the most recent version over time.
We should pick one style and enforce it with a linter to avoid the situation when individual preferences dictate how imports are coded. I don't have a strong opinion on which one to pick, but we can start with what is in this PR and switch over time to the alternative approach if folks prefer it. |
tyxla
approved these changes
Aug 7, 2023
There was a problem hiding this comment.
This looks great, CI is green and tests well in my most used couple of dev environments. Nice work @noahtallen 🚀
| import { Divider, DividerProps } from '../../divider'; | ||
| import type { WordPressComponentProps } from '../../ui/context'; | ||
| import { contextConnect } from '../../ui/context'; | ||
| import type { DividerProps } from '../../divider'; |
There was a problem hiding this comment.
Minor nit, but I've found it a little bit easier to read when import type statements are grouped below the last of the import statement.
There was a problem hiding this comment.
I like this idea, but since these were all (more or less) automatic fixes, I'm not keen on going through all the files and reordering them. Would you want to add this as a separate lint rule?
There was a problem hiding this comment.
Since we don't enforce sorting by an ESLint rule in Gutenberg, I'd abstain from this unless we can automatically sort them. It's a pretty minor deal too, feel free to ignore 😉
noahtallen
force-pushed
the
update/typescript-5
branch
from
4d8f95e
to
3ca1d3c
Compare
|
Thanks @gziolo! I tried your first suggestion, but ran out of time to figure out the second part yesterday. I appreciate the fix :) |
noahtallen
force-pushed
the
update/typescript-5
branch
from
4229d8e
to
b8749a2
Compare
|
hm, that's odd. After a simple rebase, I'm getting this error in a few jobs:
The tests pass, but something related to the cache doesn't work. |
noahtallen
force-pushed
the
update/typescript-5
branch
from
1f89640
to
74c6c86
Compare
|
It looks like this was resolved after I squashed all the package and lockfile update commits into one. Not sure why; maybe something weird with the commit sha or how it was handling the separate commits. |
This was referenced Aug 8, 2023
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.
What?
Update to Typescript 5.1. So far, seems like we need only minimal changes!
The main thing we needed to change was the
importsNotUsedAsValuesrule. This has been deprecated, and we were only using it to enforcetypeimports (see this comment for why.) It was recommended to use eslint instead if you were only using it to enforceimport typefor type imports. The new option (verbatimModuleSyntax) does several other things related to modules which probably won't work for us yet.To replace this compiler setting, I enabled the
typescript-eslint/consistent-type-importsrule. This enforces separate type imports and can be autofixed -- I also autofixed all existing reports of this rule. If folks prefer, we could enable the inline type syntax (import { type Foo } from ...), which would allow for importing types and modules in the same line.Additionally, we needed to update
typescript-eslintfrom 5.3 to 5.62 for compatibility with TS 5.1. Unfortunately, I ran into an issue, so we also have to add the two dependencies to our root devDependencies. (For some reason, npm no longer hoists this package after the update.)Testing Instructions
npm run build:package-typeslocallyimport typein ts files for type imports