Skip to content

Commit

Permalink
fix(language-service): Paths on Windows should be normalized (#40492)
Browse files Browse the repository at this point in the history
Many `ts.LanguageService` APIs accept a filename, for example
```ts
getQuickInfoAtPosition(fileName: string, position: number)
```
The requirement is that `fileName` is agnostic to the platform (Linux, Mac,
Windows, etc), and is always normalized to TypeScript's internal
`NormalizedPath`.

This is evident from the way these APIs are called from the language server:
```ts
  private onHover(params: lsp.TextDocumentPositionParams) {
    const lsInfo = this.getLSAndScriptInfo(params.textDocument);
    if (lsInfo === undefined) {
      return;
    }
    const {languageService, scriptInfo} = lsInfo;
    const offset = lspPositionToTsPosition(scriptInfo, params.position);
    const info = languageService.getQuickInfoAtPosition(scriptInfo.fileName, offset);
    // ...
  }
```
https://github.com/angular/vscode-ng-language-service/blob/9fca9c66510974c26d5c21b31adb9fa39ac0a38a/server/src/session.ts#L594
Here `scriptInfo.fileName` is always a `ts.server.NormalizedPath`.

However, #39917 accidentally leaked
the platform-specific paths, and caused a mismatch between the incoming paths
and the paths stored in the internal data structure `fileToComponent`.

This PR fixes the bug by always normalizing the paths, and updating the
type to reflect the format of the underlying data.

Fixes angular/vscode-ng-language-service#1063

PR Close #40492
  • Loading branch information
kyliau authored and AndrewKushnir committed Jan 20, 2021
1 parent 2731a4b commit b8e47e2
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 7 deletions.
13 changes: 9 additions & 4 deletions packages/language-service/src/typescript_host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,12 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
private readonly staticSymbolResolver: StaticSymbolResolver;

private readonly staticSymbolCache = new StaticSymbolCache();
private readonly fileToComponent = new Map<string, StaticSymbol>();
/**
* Key of the `fileToComponent` map must be TS internal normalized path (path
* separator must be `/`), value of the map is the StaticSymbol for the
* Component class declaration.
*/
private readonly fileToComponent = new Map<ts.server.NormalizedPath, StaticSymbol>();
private readonly collectedErrors = new Map<string, any[]>();
private readonly fileVersions = new Map<string, string>();
private readonly urlResolver: UrlResolver;
Expand Down Expand Up @@ -165,7 +170,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
/**
* Return all known external templates.
*/
getExternalTemplates(): string[] {
getExternalTemplates(): ts.server.NormalizedPath[] {
return [...this.fileToComponent.keys()];
}

Expand Down Expand Up @@ -210,7 +215,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
const templateName = this.urlResolver.resolve(
this.reflector.componentModuleUrl(directive.reference),
metadata.template.templateUrl);
this.fileToComponent.set(templateName, directive.reference);
this.fileToComponent.set(tss.server.toNormalizedPath(templateName), directive.reference);
}
}
}
Expand Down Expand Up @@ -417,7 +422,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
}
const source = snapshot.getText(0, snapshot.getLength());
// Next find the component class symbol
const classSymbol = this.fileToComponent.get(fileName);
const classSymbol = this.fileToComponent.get(tss.server.toNormalizedPath(fileName));
if (!classSymbol) {
return;
}
Expand Down
22 changes: 19 additions & 3 deletions packages/language-service/test/typescript_host_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import * as path from 'path';
import * as ts from 'typescript';

import {TypeScriptServiceHost} from '../src/typescript_host';
Expand Down Expand Up @@ -114,7 +115,7 @@ describe('TypeScriptServiceHost', () => {
const tsLS = ts.createLanguageService(tsLSHost);
const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS);
ngLSHost.getAnalyzedModules();
expect(ngLSHost.getExternalTemplates()).toContain('/app/#inner/inner.html');
expect(ngLSHost.getExternalTemplates() as string[]).toContain('/app/#inner/inner.html');
});

// https://github.com/angular/angular/issues/32301
Expand Down Expand Up @@ -238,7 +239,7 @@ describe('TypeScriptServiceHost', () => {
let content = `
import {CommonModule} from '@angular/common';
import {NgModule} from '@angular/core';
@NgModule({
entryComponents: [CommonModule],
})
Expand All @@ -256,7 +257,7 @@ describe('TypeScriptServiceHost', () => {
content = `
import {CommonModule} from '@angular/common';
import {NgModule} from '@angular/core';
@NgModule({})
export class AppModule {}
`;
Expand All @@ -265,4 +266,19 @@ describe('TypeScriptServiceHost', () => {
newModules = ngLSHost.getAnalyzedModules();
expect(newModules.ngModules.length).toBeGreaterThan(0);
});

it('should normalize path on Windows', () => {
// Spy on the `path.resolve()` method called by the URL resolver and mimic
// behavior on Windows.
spyOn(path, 'resolve').and.callFake((...pathSegments: string[]) => {
return path.win32.resolve(...pathSegments);
});
const tsLSHost = new MockTypescriptHost(['/app/main.ts']);
const tsLS = ts.createLanguageService(tsLSHost);
const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS);
ngLSHost.getAnalyzedModules();
const externalTemplates: string[] = ngLSHost.getExternalTemplates();
// External templates should be normalized.
expect(externalTemplates).toContain('/app/test.ng');
});
});

0 comments on commit b8e47e2

Please sign in to comment.