From 3b700985f0e7623b190bb1ce0aa0fb00ab159322 Mon Sep 17 00:00:00 2001 From: ayazhafiz Date: Tue, 1 Dec 2020 13:19:24 -0600 Subject: [PATCH] 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 --- .../language-service/src/typescript_host.ts | 20 ++++++++++++++----- .../test/project/app/#inner/component.ts | 16 +++++++++++++++ .../test/project/app/#inner/inner.html | 1 + .../language-service/test/project/app/main.ts | 2 ++ packages/language-service/test/test_utils.ts | 4 ++-- .../language-service/test/ts_plugin_spec.ts | 9 +++++---- .../test/typescript_host_spec.ts | 10 +++++++++- 7 files changed, 50 insertions(+), 12 deletions(-) create mode 100644 packages/language-service/test/project/app/#inner/component.ts create mode 100644 packages/language-service/test/project/app/#inner/inner.html diff --git a/packages/language-service/src/typescript_host.ts b/packages/language-service/src/typescript_host.ts index ae927fd77d66e..5bc657e0cca6e 100644 --- a/packages/language-service/src/typescript_host.ts +++ b/packages/language-service/src/typescript_host.ts @@ -6,8 +6,9 @@ * 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 * as path from 'path'; import * as tss from 'typescript/lib/tsserverlibrary'; import {createLanguageService} from './language_service'; @@ -64,6 +65,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost { private readonly fileToComponent = new Map(); private readonly collectedErrors = new Map(); private readonly fileVersions = new Map(); + private readonly urlResolver: UrlResolver; private lastProgram: tss.Program|undefined = undefined; private analyzedModules: NgAnalyzedModules = { @@ -93,6 +95,16 @@ export class TypeScriptServiceHost implements LanguageServiceHost { this.staticSymbolResolver = new StaticSymbolResolver( this.reflectorHost, this.staticSymbolCache, this.summaryResolver, (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. @@ -125,7 +137,6 @@ export class TypeScriptServiceHost implements LanguageServiceHost { const pipeResolver = new PipeResolver(staticReflector); const elementSchemaRegistry = new DomElementSchemaRegistry(); const resourceLoader = new DummyResourceLoader(); - const urlResolver = createOfflineCompileUrlResolver(); const htmlParser = new DummyHtmlParser(); // This tracks the CompileConfig in codegen.ts. Currently these options // are hard-coded. @@ -134,7 +145,7 @@ export class TypeScriptServiceHost implements LanguageServiceHost { useJit: false, }); const directiveNormalizer = - new DirectiveNormalizer(resourceLoader, urlResolver, htmlParser, config); + new DirectiveNormalizer(resourceLoader, this.urlResolver, htmlParser, config); this._resolver = new CompileMetadataResolver( config, htmlParser, moduleResolver, directiveResolver, pipeResolver, new JitSummaryResolver(), elementSchemaRegistry, directiveNormalizer, new Console(), @@ -192,12 +203,11 @@ export class TypeScriptServiceHost implements LanguageServiceHost { } // update template references and fileToComponent - const urlResolver = createOfflineCompileUrlResolver(); for (const ngModule of this.analyzedModules.ngModules) { for (const directive of ngModule.declaredDirectives) { const {metadata} = this.resolver.getNonNormalizedDirectiveMetadata(directive.reference)!; if (metadata.isComponent && metadata.template && metadata.template.templateUrl) { - const templateName = urlResolver.resolve( + const templateName = this.urlResolver.resolve( this.reflector.componentModuleUrl(directive.reference), metadata.template.templateUrl); this.fileToComponent.set(templateName, directive.reference); diff --git a/packages/language-service/test/project/app/#inner/component.ts b/packages/language-service/test/project/app/#inner/component.ts new file mode 100644 index 0000000000000..59a256721bd80 --- /dev/null +++ b/packages/language-service/test/project/app/#inner/component.ts @@ -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 { +} diff --git a/packages/language-service/test/project/app/#inner/inner.html b/packages/language-service/test/project/app/#inner/inner.html new file mode 100644 index 0000000000000..0fd9beb678ff4 --- /dev/null +++ b/packages/language-service/test/project/app/#inner/inner.html @@ -0,0 +1 @@ +
Hello
diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts index 4860cc3930c91..0599c9a523c6b 100644 --- a/packages/language-service/test/project/app/main.ts +++ b/packages/language-service/test/project/app/main.ts @@ -9,6 +9,7 @@ import {CommonModule} from '@angular/common'; import {NgModule} from '@angular/core'; import {FormsModule} from '@angular/forms'; +import {InnerComponent} from './#inner/component'; import {AppComponent} from './app.component'; import * as ParsingCases from './parsing-cases'; @@ -16,6 +17,7 @@ import * as ParsingCases from './parsing-cases'; imports: [CommonModule, FormsModule], declarations: [ AppComponent, + InnerComponent, ParsingCases.CounterDirective, ParsingCases.HintModel, ParsingCases.NumberModel, diff --git a/packages/language-service/test/test_utils.ts b/packages/language-service/test/test_utils.ts index 0be4ff12cfa4c..39e7497e0a2fc 100644 --- a/packages/language-service/test/test_utils.ts +++ b/packages/language-service/test/test_utils.ts @@ -69,7 +69,7 @@ function loadTourOfHeroes(): ReadonlyMap { const value = fs.readFileSync(absPath, 'utf8'); files.set(key, value); } else { - const key = path.join('/', filePath); + const key = path.join('/', path.relative(root, absPath)); files.set(key, '[[directory]]'); dirs.push(absPath); } @@ -189,7 +189,7 @@ export class MockTypescriptHost implements ts.LanguageServiceHost { if (this.overrideDirectory.has(directoryName)) return true; const effectiveName = this.getEffectiveName(directoryName); if (effectiveName === directoryName) { - return TOH.has(directoryName); + return TOH.get(directoryName) === '[[directory]]'; } if (effectiveName === '/' + this.node_modules) { return true; diff --git a/packages/language-service/test/ts_plugin_spec.ts b/packages/language-service/test/ts_plugin_spec.ts index 099146c6aedd7..a6a4e506b895c 100644 --- a/packages/language-service/test/ts_plugin_spec.ts +++ b/packages/language-service/test/ts_plugin_spec.ts @@ -58,8 +58,8 @@ describe('plugin', () => { const compilerDiags = tsLS.getCompilerOptionsDiagnostics(); expect(compilerDiags).toEqual([]); const sourceFiles = program.getSourceFiles().filter(f => !f.fileName.endsWith('.d.ts')); - // there are three .ts files in the test project - expect(sourceFiles.length).toBe(3); + // there are four .ts files in the test project + expect(sourceFiles.length).toBe(4); for (const {fileName} of sourceFiles) { const syntacticDiags = tsLS.getSyntacticDiagnostics(fileName); expect(syntacticDiags).toEqual([]); @@ -133,9 +133,10 @@ describe('plugin', () => { it('should return external templates when getExternalFiles() is called', () => { const externalTemplates = getExternalFiles(mockProject); - expect(externalTemplates).toEqual([ + expect(new Set(externalTemplates)).toEqual(new Set([ '/app/test.ng', - ]); + '/app/#inner/inner.html', + ])); }); it('should not return external template that does not exist', () => { diff --git a/packages/language-service/test/typescript_host_spec.ts b/packages/language-service/test/typescript_host_spec.ts index 4719150fdbb95..f1bab4d1bc34d 100644 --- a/packages/language-service/test/typescript_host_spec.ts +++ b/packages/language-service/test/typescript_host_spec.ts @@ -6,7 +6,6 @@ * found in the LICENSE file at https://angular.io/license */ -import * as ngc from '@angular/compiler'; import * as ts from 'typescript'; import {TypeScriptServiceHost} from '../src/typescript_host'; @@ -109,6 +108,15 @@ describe('TypeScriptServiceHost', () => { expect(template.source).toContain('

{{hero.name}} details!

'); }); + // 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 it('should clear caches when program changes', () => { const tsLSHost = new MockTypescriptHost(['/app/main.ts']);