-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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(core): add support for TypeScript 5.0 #49126
Conversation
1793668
to
c050f3b
Compare
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.
LGTM, just a few small questions on additional context.
@@ -5,7 +5,7 @@ | |||
// TODO: Work out how to fix the broken segment for the last item in a template | |||
.ɵɵelem // SOURCE: "/ng_if_simple.ts" "</div>'" | |||
… | |||
.ɵɵtextInterpolate(ctx_r0.name);\n }\n}\n\n // SOURCE: "/ng_if_simple.ts" "{{ name }}" | |||
.ɵɵtextInterpolate(ctx_r0.name);\n }\n}\n\nclass // SOURCE: "/ng_if_simple.ts" "{{ name }}" |
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.
Does this mean that the source mappings are incorrect? well, I guess, they were a little incorrect anyway if they included the \n}\n
?
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.
tbh, I'm not sure, as you said a correct mapping wouldn't include the newlines. It's possible that this is just how our testing utilities work.
readonly AT_TARGET = 2; | ||
readonly BUBBLING_PHASE = 3; | ||
readonly CAPTURING_PHASE = 1; | ||
readonly NONE = 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.
Is there any context on this change? (if you recall, can be in the comment here)
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 internal typings changed from readonly NONE: number
to readonly NONE: 0
so our mock had to be updated as well.
packages/compiler-cli/package.json
Outdated
@@ -53,7 +53,7 @@ | |||
}, | |||
"peerDependencies": { | |||
"@angular/compiler": "0.0.0-PLACEHOLDER", | |||
"typescript": ">=4.8.2 <5.0" | |||
"typescript": ">=4.8.2 <5.1" |
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.
Just picking this place for the comment: Are there things we need to keep in mind with the decorator changes. i.e. when applications don't have --experimentalDecorators=true
? https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#differences-with-experimental-legacy-decorators
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.
For the moment if apps disable experimentalDecorators
, they'll get a compilation error on parameter decorators before the code even reaches our compiler. I think that eventually we'll have a schematic to move apps away from constructor injection.
Updates the project to support TypeScript 5.0 and to resolve any errors that came up as a result of the update.
This PR was merged into the repository by commit 99d874f. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Updates the project to support TypeScript 5.0 and to resolve any errors that came up as a result of the update.