fix(language-service): return all typecheck files via getExternalFiles #40162

kyliau · 2020-12-16T22:16:31Z

We need a means to preserve typecheck files when a project is reloaded,
otherwise the Ivy compiler will throw an error when it fails to find them in the program.
(The compiler is told by the IncrementalDriver that the tcfs exist.)
This commit implements getExternalFiles() called by the language
server to achieve this goal.

For more info see angular/vscode-ng-language-service#1030

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

kyliau · 2020-12-16T22:40:45Z

packages/language-service/ivy/ts_plugin.ts

+export function getExternalFiles(project: ts.server.Project): string[] {
+  const typecheckFiles: string[] = [];
+  for (const scriptInfo of project.getScriptInfos()) {
+    if (scriptInfo.scriptKind === ts.ScriptKind.External) {


Initially I tried to get tcfs via source files in the program, but it's just way too complex and not worth the effort. The isShim() method does not work, because according to Andrew, the source file tagged by the compiler is not the same as the source file contained in the latest program. The other method, templateTypeChecker. isTrackedTypeCheckFile() involves instantiating the Ivy compiler and the ttc, so it's also quite involved.
We have three different methods to detect typecheck files now, so we should probably do a cleanup in the future.

Do you know if we'll have issues with this check if other plugins also register external files?

Not in the way this package (@angular/language-service) is used, because we control the server, and we know for sure the Angular language service is the only plugin installed.

atscott · 2020-12-16T22:52:20Z

packages/language-service/ivy/language_service.ts

+        '',    // fileContent
+        // script info added by plugins should be marked as external, see
+        // https://github.com/microsoft/TypeScript/blob/b217f22e798c781f55d17da72ed099a9dee5c650/src/compiler/program.ts#L1897-L1899
+        ts.ScriptKind.External,  // scriptKind


Hmm, I wonder if this has implications for "find references". Have you tried building the plugin with this change and running it in the extension host?

yes, I did, and I can confirm that "find references" still works. From TS language service point of view, the script info kind doesn't really matter, whether they are TS or External, they all end up in the ts.Program (see code in the link above), so I doubt it'll affect "find references".

atscott · 2020-12-16T22:56:05Z

packages/language-service/ivy/ts_plugin.ts

+export function getExternalFiles(project: ts.server.Project): string[] {
+  const typecheckFiles: string[] = [];
+  for (const scriptInfo of project.getScriptInfos()) {
+    if (scriptInfo.scriptKind === ts.ScriptKind.External) {


Do you know if we'll have issues with this check if other plugins also register external files?

atscott · 2020-12-16T22:58:51Z

packages/language-service/ivy/test/legacy/ts_plugin_spec.ts

+    // Now that global analysis is run, we should have all the typecheck files
+    externalFiles = getExternalFiles(project);
+    expect(externalFiles.length).toBe(1);
+    expect(externalFiles[0].endsWith('app.component.ngtypecheck.ts')).toBeTrue();


Is there a more e2e way that we can test this? Like triggering a reload on the project, either with a direct call to updateGraph or doing some other action that would trigger it organically and then asserting that our stuff still works?

Maybe a test like that is dependent on this PR being submitted first (since the e2e tests in the client depend on the published version of the plugin).

There are some integration tests for the VE-based language service in this repo. I think a test to make sure that the typeck files are kept after a project reload are a good idea, but should be done in angular/angular if we're going to do them.

atscott

👍

ayazhafiz · 2020-12-17T01:07:28Z

packages/language-service/ivy/test/legacy/ts_plugin_spec.ts

+    // Now that global analysis is run, we should have all the typecheck files
+    externalFiles = getExternalFiles(project);
+    expect(externalFiles.length).toBe(1);
+    expect(externalFiles[0].endsWith('app.component.ngtypecheck.ts')).toBeTrue();


There are some integration tests for the VE-based language service in this repo. I think a test to make sure that the typeck files are kept after a project reload are a good idea, but should be done in angular/angular if we're going to do them.

This is a follow-up to angular/angular#40162. In View Engine, `getExternalFiles()` returns a list of external templates (HTML files), but we should not have marked them as `ScriptKind.External`. As noted in https://github.com/microsoft/TypeScript/blob/b217f22e798c781f55d17da72ed099a9dee5c650/src/compiler/program.ts#L1897-L1899 `ScriptKind.External` should be used for external TypeScript files added by plugins. External templates should be marked as `ScriptKind.Unknown` instead.

We need a means to preserve typecheck files when a project is reloaded, otherwise the Ivy compiler will throw an error when it's unable to find them. This commit implements `getExternalFiles()` called by the langauge server to achieve this goal. For more info see angular/vscode-ng-language-service#1030

kyliau · 2021-01-06T18:57:31Z

I'll merge this for now. Created https://github.com/orgs/angular/projects/1#card-52319541 to keep track of the integration test for project reload.

angular-automatic-lock-bot · 2021-02-06T16:34:07Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Dec 16, 2020

kyliau added area: language-service Issues related to Angular's VS Code language service and removed cla: yes labels Dec 16, 2020

ngbot bot modified the milestone: Backlog Dec 16, 2020

google-cla bot added the cla: yes label Dec 16, 2020

kyliau added the target: patch This PR is targeted for the next patch release label Dec 16, 2020

kyliau force-pushed the get-external-tcf branch from a909851 to cfe3ca0 Compare December 16, 2020 22:29

kyliau marked this pull request as ready for review December 16, 2020 22:30

kyliau requested review from ayazhafiz and atscott December 16, 2020 22:30

kyliau commented Dec 16, 2020

View reviewed changes

atscott reviewed Dec 16, 2020

View reviewed changes

atscott approved these changes Dec 16, 2020

View reviewed changes

ayazhafiz approved these changes Dec 17, 2020

View reviewed changes

kyliau mentioned this pull request Dec 17, 2020

fix: set external templates as ScriptKind.unknown angular/vscode-ng-language-service#1037

Merged

kyliau force-pushed the get-external-tcf branch from cfe3ca0 to ff7260c Compare December 17, 2020 17:47

kyliau force-pushed the get-external-tcf branch from ff7260c to 106ef3e Compare December 21, 2020 23:07

kyliau force-pushed the get-external-tcf branch from 106ef3e to 3883a0c Compare January 6, 2021 18:54

kyliau added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 6, 2021

kyliau added the action: merge The PR is ready for merge by the caretaker label Jan 6, 2021

josephperrott closed this in 183fb7e Jan 6, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(language-service): return all typecheck files via getExternalFiles #40162

fix(language-service): return all typecheck files via getExternalFiles #40162

kyliau commented Dec 16, 2020

kyliau Dec 16, 2020

atscott Dec 16, 2020

kyliau Dec 16, 2020

atscott Dec 16, 2020

kyliau Dec 16, 2020

atscott Dec 16, 2020

atscott Dec 16, 2020

ayazhafiz Dec 17, 2020

atscott left a comment

ayazhafiz Dec 17, 2020

kyliau commented Jan 6, 2021

angular-automatic-lock-bot bot commented Feb 6, 2021

fix(language-service): return all typecheck files via getExternalFiles #40162

fix(language-service): return all typecheck files via getExternalFiles #40162

Conversation

kyliau commented Dec 16, 2020

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyliau commented Jan 6, 2021

angular-automatic-lock-bot bot commented Feb 6, 2021