Conversation
cd2286b to
46c0725
Compare
Updates the project to support TypeScript 5.2.
| 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:
| // 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
| 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.
|
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.