-
-
Notifications
You must be signed in to change notification settings - Fork 32
fix: update TypeScript to v5 #112
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
Conversation
This allow support of Typescript 5, that were fixed in nestjs/nest#10970 and merged in 9.3.0
Quoting the release note (https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/) > But if you’re already familiar with TypeScript, have no fear!5.0 is not a disruptive release, and everything > you know is still applicable.While TypeScript 5.0 includes correctness changes and some deprecations for > infrequently-used options, we believe most developers will have an upgrade experience similar to previous > releases. It still supports experimental-legacy-decorators: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#differences-with-experimental-legacy-decorators DEPRECIATIONS: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0/#deprecations-and-default-changes suppressImplicitAnyIndexErrors is deprecated and will be removed in 5.5. Until, this we can use "ignoreDeprecations" to mask the error.
Was upgraded in mikro-orm/mikro-orm#177 in the readme but not the contributing.md
Before running yarn install and seeing big changes in lockfile, it wasn't clear that yarn 3.x was used. It is now part of the contributing guide.
@@ -7,6 +7,7 @@ | |||
"declaration": true, | |||
"sourceMap": false, | |||
"strict": true, | |||
"ignoreDeprecations": "5.0", |
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.
better to fix this instead of ignoring it. it should be pretty simple given the project scope.
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.
Have you actually tried to build the project before you added this? It looks like there are no issues without it.
edit: my bad, forgot to install the new TS dependency 🤦 still, what you should have done is to remove the suppressImplicitAnyIndexErrors
option, we apparently no longer need it in the codebase, and given its deprecated, no reason to use it.
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.
Yes build used to failed with typescript 5.0.
Fine for fixing it rather than ignoring issue.
I just copied what done in the main repo with the mto in mind "the more different stuff you do in a pr, the longer it will take to merge.
I guess I should have tried a tad more 😀
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.
Thanks!
Thank for merging it. |
What problem am I fixing
Recently, Typescript 5.0 was released.
Type checking decorator used to be buggy [1], and that was fixed in part of the "Implement Stage3 Decorator proposal" [2].
Sadly, it means that the type checking is now more strict, and new error pops.
On a real project, that means:
NestJS fixed it in version 9.3.0 (see issue [3] and commit [4]).
This PR upgrade nest dependencies.
What is done
The work is done in 4 atomics commit. If you wich, you can safely remove any one of those before merging or cherry pick them on another PR if it is clearer.
Test plan
Conclusing
Let me know if i can do other fixes and thank you for the great works on mikro orm.
Links
[1] microsoft/TypeScript#52435 (comment)
[2] microsoft/TypeScript#50820
[3] nestjs/nest#10959
[4] nestjs/nest@268d930