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 angular/vscode-ng-language-service#892 Closes angular/vscode-ng-language-service#1001 PR Close #39917
angular · Dec 3, 2020 · 829988b · 829988b
1 parent 35309bb
commit 829988b
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 12 deletions.
diff --git 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<string, StaticSymbol>();
   private readonly collectedErrors = new Map<string, any[]>();
   private readonly fileVersions = new Map<string, string>();
+  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
@@ -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
@@ -0,0 +1 @@
+<div>Hello</div>
diff --git a/packages/language-service/test/project/app/main.ts b/packages/language-service/test/project/app/main.ts
@@ -9,13 +9,15 @@
 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';
 
 @NgModule({
   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
@@ -69,7 +69,7 @@ function loadTourOfHeroes(): ReadonlyMap<string, string> {
         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
@@ -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
@@ -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('<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
   it('should clear caches when program changes', () => {
     const tsLSHost = new MockTypescriptHost(['/app/main.ts']);