Skip to content

Commit

Permalink
fix(language-service): do not treat file URIs as general URLs (#39917)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ayazhafiz authored and mhevery committed Dec 3, 2020
1 parent 35309bb commit 829988b
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 12 deletions.
20 changes: 15 additions & 5 deletions packages/language-service/src/typescript_host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand All @@ -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(),
Expand Down Expand Up @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions packages/language-service/test/project/app/#inner/component.ts
Original file line number Diff line number Diff line change
@@ -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 {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<div>Hello</div>
2 changes: 2 additions & 0 deletions packages/language-service/test/project/app/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions packages/language-service/test/test_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 5 additions & 4 deletions packages/language-service/test/ts_plugin_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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([]);
Expand Down Expand Up @@ -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', () => {
Expand Down
10 changes: 9 additions & 1 deletion packages/language-service/test/typescript_host_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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']);
Expand Down

0 comments on commit 829988b

Please sign in to comment.