feat(core): support TypeScript 5.2 #51334
Conversation
crisbeto
force-pushed
the
ts-5.2
branch
3 times, most recently
from
cd2286b to
46c0725
Compare
Updates the project to support TypeScript 5.2.
| @@ -306,21 +306,27 @@ describe('type definitions', () => { | |||
| const definitions = getTypeDefinitions({ | |||
| templateOverride: `<div *ngFor="let item of heroes as her¦oes2; trackBy: test;"></div>`, | |||
| }); | |||
| expectAllDefinitions(definitions, new Set(['Array']), possibleArrayDefFiles); | |||
| expectAllDefinitions( | |||
There was a problem hiding this comment.
Note for reviewer: this test had a legitimate behavior difference where the returned definition is now ['Hero', 'Array'], instead of ['Array']. As far as I can tell, this has nothing to do with our implementation since it's basically a direct call into the TS language service here:
crisbeto
added
action: review
| // TODO: temporarily disabled until ESM issue is resolved. | ||
| // import * as compilerCli from '@angular/compiler-cli'; | ||
| // import * as localize from '@angular/localize'; | ||
|
|
There was a problem hiding this comment.
cc @devversion since this had to be disabled due to compiler-cli and localize not using ESM.
There was a problem hiding this comment.
It's good that we have this captured in code now.
There was a problem hiding this comment.
For later context: We knew this was always not fully compatible- the test just didn't show this because we had moduleResolution still set to node and not nodenext
| // TODO: temporarily disabled until ESM issue is resolved. | ||
| // import * as compilerCli from '@angular/compiler-cli'; | ||
| // import * as localize from '@angular/localize'; | ||
|
|
There was a problem hiding this comment.
It's good that we have this captured in code now.
| // TODO: temporarily disabled until ESM issue is resolved. | ||
| // import * as compilerCli from '@angular/compiler-cli'; | ||
| // import * as localize from '@angular/localize'; | ||
|
|
There was a problem hiding this comment.
For later context: We knew this was always not fully compatible- the test just didn't show this because we had moduleResolution still set to node and not nodenext
| @@ -32,6 +32,11 @@ export class OpenBuffer { | |||
| set contents(newContents: string) { | |||
| const snapshot = this.scriptInfo.getSnapshot(); | |||
| this.scriptInfo.editContent(0, snapshot.getLength(), newContents); | |||
|
|
|||
| // As of TypeScript 5.2 we need to trigger graph update manually in order to satisfy the | |||
There was a problem hiding this comment.
Not an expert on the LS, but the comment on the assertion. I can't fully reason where we end up hitting this assertion in the context of set contents?
There was a problem hiding this comment.
We hit it on the specific line in the link. In the set contents we have a call to ts.server.ScriptInfo.editContent which goes through a few other methods before hitting updateGraphWorker. My understanding is that because the program shape might've changed, it need to be recomputed.
There was a problem hiding this comment.
I wonder how expensive updateGraph is when the program shape hasn't changed.
There was a problem hiding this comment.
Capturing what I mentioned over DM: this code is only used in tests so even if there is some overhead, it's not as big of a deal.
crisbeto
added
action: merge
|
This PR was merged into the repository by commit 9cc52b9. |
|
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.2. PR Close angular#51334
Updates the project to support TypeScript 5.2. PR Close angular#51334
Updates the project to support TypeScript 5.2.