fix(language-service): do not treat file URIs as general URLs (#39917)

In the past, the legacy (VE-based) language service would use a
`UrlResolver` instance to resolve file paths, primarily for compiler
resources like external templates. The problem with this is that the
UrlResolver is designed to resolve URLs in general, and so for a path
like `/a/b/#c`, `#c` is treated as hash/fragment rather than as part
of the path, which can lead to unexpected path resolution (f.x.,
`resolve('a/b/#c/d.ts', './d.html')` would produce `'a/b/d.html'` rather
than the expected `'a/b/#c/d.html'`).

This commit resolves the issue by using Node's `path` module to resolve
file paths directly, which aligns more with how resources are resolved
in the Ivy compiler.

The testing story here is not great, and the API for validating a file
path could be a little bit prettier/robust. However, since the VE-based
language service is going into more of a "maintenance mode" now that
there is a clear path for the Ivy-based LS moving forward, I think it is
okay not to spend too much time here.

Closes https://github.com/angular/vscode-ng-language-service/issues/892
Closes https://github.com/angular/vscode-ng-language-service/issues/1001

PR Close #39917
This commit is contained in:
ayazhafiz 2020-12-01 13:19:24 -06:00 committed by Misko Hevery
parent dd18cfd983
commit 3b700985f0
7 changed files with 50 additions and 12 deletions

View File

@ -6,8 +6,9 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import {analyzeNgModules, AotSummaryResolver, CompileDirectiveSummary, CompileMetadataResolver, CompileNgModuleMetadata, CompilePipeSummary, CompilerConfig, createOfflineCompileUrlResolver, DirectiveNormalizer, DirectiveResolver, DomElementSchemaRegistry, FormattedError, FormattedMessageChain, HtmlParser, isFormattedError, JitSummaryResolver, Lexer, NgAnalyzedModules, NgModuleResolver, Parser, ParseTreeResult, PipeResolver, ResourceLoader, StaticReflector, StaticSymbol, StaticSymbolCache, StaticSymbolResolver, TemplateParser} from '@angular/compiler'; import {analyzeNgModules, AotSummaryResolver, CompileDirectiveSummary, CompileMetadataResolver, CompileNgModuleMetadata, CompilePipeSummary, CompilerConfig, DirectiveNormalizer, DirectiveResolver, DomElementSchemaRegistry, FormattedError, FormattedMessageChain, HtmlParser, isFormattedError, JitSummaryResolver, Lexer, NgAnalyzedModules, NgModuleResolver, Parser, ParseTreeResult, PipeResolver, ResourceLoader, StaticReflector, StaticSymbol, StaticSymbolCache, StaticSymbolResolver, TemplateParser, UrlResolver} from '@angular/compiler';
import {SchemaMetadata, ViewEncapsulation, ɵConsole as Console} from '@angular/core'; import {SchemaMetadata, ViewEncapsulation, ɵConsole as Console} from '@angular/core';
import * as path from 'path';
import * as tss from 'typescript/lib/tsserverlibrary'; import * as tss from 'typescript/lib/tsserverlibrary';
import {createLanguageService} from './language_service'; import {createLanguageService} from './language_service';
@ -64,6 +65,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
private readonly fileToComponent = new Map<string, StaticSymbol>(); private readonly fileToComponent = new Map<string, StaticSymbol>();
private readonly collectedErrors = new Map<string, any[]>(); private readonly collectedErrors = new Map<string, any[]>();
private readonly fileVersions = new Map<string, string>(); private readonly fileVersions = new Map<string, string>();
private readonly urlResolver: UrlResolver;
private lastProgram: tss.Program|undefined = undefined; private lastProgram: tss.Program|undefined = undefined;
private analyzedModules: NgAnalyzedModules = { private analyzedModules: NgAnalyzedModules = {
@ -93,6 +95,16 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
this.staticSymbolResolver = new StaticSymbolResolver( this.staticSymbolResolver = new StaticSymbolResolver(
this.reflectorHost, this.staticSymbolCache, this.summaryResolver, this.reflectorHost, this.staticSymbolCache, this.summaryResolver,
(e, filePath) => this.collectError(e, filePath)); (e, filePath) => this.collectError(e, filePath));
this.urlResolver = {
resolve: (baseUrl: string, url: string) => {
// In practice, `directoryExists` is always defined.
// https://github.com/microsoft/TypeScript/blob/0b6c9254a850dd07056259d4eefca7721745af75/src/server/project.ts#L1608-L1614
if (tsLsHost.directoryExists!(baseUrl)) {
return path.resolve(baseUrl, url);
}
return path.resolve(path.dirname(baseUrl), url);
}
};
} }
// The resolver is instantiated lazily and should not be accessed directly. // The resolver is instantiated lazily and should not be accessed directly.
@ -125,7 +137,6 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
const pipeResolver = new PipeResolver(staticReflector); const pipeResolver = new PipeResolver(staticReflector);
const elementSchemaRegistry = new DomElementSchemaRegistry(); const elementSchemaRegistry = new DomElementSchemaRegistry();
const resourceLoader = new DummyResourceLoader(); const resourceLoader = new DummyResourceLoader();
const urlResolver = createOfflineCompileUrlResolver();
const htmlParser = new DummyHtmlParser(); const htmlParser = new DummyHtmlParser();
// This tracks the CompileConfig in codegen.ts. Currently these options // This tracks the CompileConfig in codegen.ts. Currently these options
// are hard-coded. // are hard-coded.
@ -134,7 +145,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
useJit: false, useJit: false,
}); });
const directiveNormalizer = const directiveNormalizer =
new DirectiveNormalizer(resourceLoader, urlResolver, htmlParser, config); new DirectiveNormalizer(resourceLoader, this.urlResolver, htmlParser, config);
this._resolver = new CompileMetadataResolver( this._resolver = new CompileMetadataResolver(
config, htmlParser, moduleResolver, directiveResolver, pipeResolver, config, htmlParser, moduleResolver, directiveResolver, pipeResolver,
new JitSummaryResolver(), elementSchemaRegistry, directiveNormalizer, new Console(), new JitSummaryResolver(), elementSchemaRegistry, directiveNormalizer, new Console(),
@ -192,12 +203,11 @@ export class TypeScriptServiceHost implements LanguageServiceHost {
} }
// update template references and fileToComponent // update template references and fileToComponent
const urlResolver = createOfflineCompileUrlResolver();
for (const ngModule of this.analyzedModules.ngModules) { for (const ngModule of this.analyzedModules.ngModules) {
for (const directive of ngModule.declaredDirectives) { for (const directive of ngModule.declaredDirectives) {
const {metadata} = this.resolver.getNonNormalizedDirectiveMetadata(directive.reference)!; const {metadata} = this.resolver.getNonNormalizedDirectiveMetadata(directive.reference)!;
if (metadata.isComponent && metadata.template && metadata.template.templateUrl) { if (metadata.isComponent && metadata.template && metadata.template.templateUrl) {
const templateName = urlResolver.resolve( const templateName = this.urlResolver.resolve(
this.reflector.componentModuleUrl(directive.reference), this.reflector.componentModuleUrl(directive.reference),
metadata.template.templateUrl); metadata.template.templateUrl);
this.fileToComponent.set(templateName, directive.reference); this.fileToComponent.set(templateName, directive.reference);

View File

@ -0,0 +1,16 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {Component} from '@angular/core';
@Component({
selector: 'inner',
templateUrl: './inner.html',
})
export class InnerComponent {
}

View File

@ -0,0 +1 @@
<div>Hello</div>

View File

@ -9,6 +9,7 @@
import {CommonModule} from '@angular/common'; import {CommonModule} from '@angular/common';
import {NgModule} from '@angular/core'; import {NgModule} from '@angular/core';
import {FormsModule} from '@angular/forms'; import {FormsModule} from '@angular/forms';
import {InnerComponent} from './#inner/component';
import {AppComponent} from './app.component'; import {AppComponent} from './app.component';
import * as ParsingCases from './parsing-cases'; import * as ParsingCases from './parsing-cases';
@ -16,6 +17,7 @@ import * as ParsingCases from './parsing-cases';
imports: [CommonModule, FormsModule], imports: [CommonModule, FormsModule],
declarations: [ declarations: [
AppComponent, AppComponent,
InnerComponent,
ParsingCases.CounterDirective, ParsingCases.CounterDirective,
ParsingCases.HintModel, ParsingCases.HintModel,
ParsingCases.NumberModel, ParsingCases.NumberModel,

View File

@ -69,7 +69,7 @@ function loadTourOfHeroes(): ReadonlyMap<string, string> {
const value = fs.readFileSync(absPath, 'utf8'); const value = fs.readFileSync(absPath, 'utf8');
files.set(key, value); files.set(key, value);
} else { } else {
const key = path.join('/', filePath); const key = path.join('/', path.relative(root, absPath));
files.set(key, '[[directory]]'); files.set(key, '[[directory]]');
dirs.push(absPath); dirs.push(absPath);
} }
@ -189,7 +189,7 @@ export class MockTypescriptHost implements ts.LanguageServiceHost {
if (this.overrideDirectory.has(directoryName)) return true; if (this.overrideDirectory.has(directoryName)) return true;
const effectiveName = this.getEffectiveName(directoryName); const effectiveName = this.getEffectiveName(directoryName);
if (effectiveName === directoryName) { if (effectiveName === directoryName) {
return TOH.has(directoryName); return TOH.get(directoryName) === '[[directory]]';
} }
if (effectiveName === '/' + this.node_modules) { if (effectiveName === '/' + this.node_modules) {
return true; return true;

View File

@ -58,8 +58,8 @@ describe('plugin', () => {
const compilerDiags = tsLS.getCompilerOptionsDiagnostics(); const compilerDiags = tsLS.getCompilerOptionsDiagnostics();
expect(compilerDiags).toEqual([]); expect(compilerDiags).toEqual([]);
const sourceFiles = program.getSourceFiles().filter(f => !f.fileName.endsWith('.d.ts')); const sourceFiles = program.getSourceFiles().filter(f => !f.fileName.endsWith('.d.ts'));
// there are three .ts files in the test project // there are four .ts files in the test project
expect(sourceFiles.length).toBe(3); expect(sourceFiles.length).toBe(4);
for (const {fileName} of sourceFiles) { for (const {fileName} of sourceFiles) {
const syntacticDiags = tsLS.getSyntacticDiagnostics(fileName); const syntacticDiags = tsLS.getSyntacticDiagnostics(fileName);
expect(syntacticDiags).toEqual([]); expect(syntacticDiags).toEqual([]);
@ -133,9 +133,10 @@ describe('plugin', () => {
it('should return external templates when getExternalFiles() is called', () => { it('should return external templates when getExternalFiles() is called', () => {
const externalTemplates = getExternalFiles(mockProject); const externalTemplates = getExternalFiles(mockProject);
expect(externalTemplates).toEqual([ expect(new Set(externalTemplates)).toEqual(new Set([
'/app/test.ng', '/app/test.ng',
]); '/app/#inner/inner.html',
]));
}); });
it('should not return external template that does not exist', () => { it('should not return external template that does not exist', () => {

View File

@ -6,7 +6,6 @@
* found in the LICENSE file at https://angular.io/license * found in the LICENSE file at https://angular.io/license
*/ */
import * as ngc from '@angular/compiler';
import * as ts from 'typescript'; import * as ts from 'typescript';
import {TypeScriptServiceHost} from '../src/typescript_host'; import {TypeScriptServiceHost} from '../src/typescript_host';
@ -109,6 +108,15 @@ describe('TypeScriptServiceHost', () => {
expect(template.source).toContain('<h2>{{hero.name}} details!</h2>'); expect(template.source).toContain('<h2>{{hero.name}} details!</h2>');
}); });
// https://github.com/angular/vscode-ng-language-service/issues/892
it('should resolve external templates with `#` in the path', () => {
const tsLSHost = new MockTypescriptHost(['/app/main.ts']);
const tsLS = ts.createLanguageService(tsLSHost);
const ngLSHost = new TypeScriptServiceHost(tsLSHost, tsLS);
ngLSHost.getAnalyzedModules();
expect(ngLSHost.getExternalTemplates()).toContain('/app/#inner/inner.html');
});
// https://github.com/angular/angular/issues/32301 // https://github.com/angular/angular/issues/32301
it('should clear caches when program changes', () => { it('should clear caches when program changes', () => {
const tsLSHost = new MockTypescriptHost(['/app/main.ts']); const tsLSHost = new MockTypescriptHost(['/app/main.ts']);