Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compiler-cli): Allow analysis to continue with invalid style url #41403

Closed
wants to merge 2 commits into from
Closed

fix(compiler-cli): Allow analysis to continue with invalid style url #41403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fe6bda
fix(compiler-cli): Allow analysis to continue with invalid style url
atscott Mar 31, 2021
d0b2af3
fixup! fix(compiler-cli): Allow analysis to continue with invalid sty…
atscott Apr 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
199 changes: 104 additions & 95 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {compileComponentFromMetadata, compileDeclareComponentFromMetadata, Const
import * as ts from 'typescript';

import {Cycle, CycleAnalyzer, CycleHandlingStrategy} from '../../cycles';
import {ErrorCode, FatalDiagnosticError, makeRelatedInformation} from '../../diagnostics';
import {ErrorCode, FatalDiagnosticError, makeDiagnostic, makeRelatedInformation} from '../../diagnostics';
import {absoluteFrom, relative} from '../../file_system';
import {DefaultImportRecorder, ImportedFile, ModuleResolver, Reference, ReferenceEmitter} from '../../imports';
import {DependencyTracker} from '../../incremental/api';
Expand Down Expand Up @@ -262,13 +262,16 @@ export class ComponentDecoratorHandler implements
const component = reflectObjectLiteral(meta);
const containingFile = node.getSourceFile().fileName;

const resolveStyleUrl =
(styleUrl: string, nodeForError: ts.Node,
resourceType: ResourceTypeForDiagnostics): Promise<void>|undefined => {
const resourceUrl =
this._resolveResourceOrThrow(styleUrl, containingFile, nodeForError, resourceType);
return this.resourceLoader.preload(resourceUrl, {type: 'style', containingFile});
};
const resolveStyleUrl = (styleUrl: string): Promise<void>|undefined => {
try {
const resourceUrl = this.resourceLoader.resolve(styleUrl, containingFile);
return this.resourceLoader.preload(resourceUrl, {type: 'style', containingFile});
} catch {
// Don't worry about failures to preload. We can handle this problem during analysis by
// producing a diagnostic.
return undefined;
}
};

// A Promise that waits for the template and all <link>ed styles within it to be preloaded.
const templateAndTemplateStyleResources =
Expand All @@ -278,12 +281,7 @@ export class ComponentDecoratorHandler implements
return undefined;
}

const nodeForError = getTemplateDeclarationNodeForError(template.declaration);
return Promise
.all(template.styleUrls.map(
styleUrl => resolveStyleUrl(
styleUrl, nodeForError,
ResourceTypeForDiagnostics.StylesheetFromTemplate)))
return Promise.all(template.styleUrls.map(styleUrl => resolveStyleUrl(styleUrl)))
.then(() => undefined);
});

Expand Down Expand Up @@ -311,10 +309,7 @@ export class ComponentDecoratorHandler implements
return Promise
.all([
templateAndTemplateStyleResources, inlineStyles,
...componentStyleUrls.map(
styleUrl => resolveStyleUrl(
styleUrl.url, styleUrl.nodeForError,
ResourceTypeForDiagnostics.StylesheetFromDecorator))
...componentStyleUrls.map(styleUrl => resolveStyleUrl(styleUrl.url))
])
.then(() => undefined);
}
Expand All @@ -326,6 +321,8 @@ export class ComponentDecoratorHandler implements
const containingFile = node.getSourceFile().fileName;
this.literalCache.delete(decorator);

let diagnostics: ts.Diagnostic[]|undefined;
let isPoisoned = false;
// @Component inherits @Directive, so begin by extracting the @Directive metadata and building
// on it.
const directiveResult = extractDirectiveMetadata(
Expand Down Expand Up @@ -408,16 +405,24 @@ export class ComponentDecoratorHandler implements
];

for (const styleUrl of styleUrls) {
const resourceType = styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ?
ResourceTypeForDiagnostics.StylesheetFromDecorator :
ResourceTypeForDiagnostics.StylesheetFromTemplate;
const resourceUrl = this._resolveResourceOrThrow(
styleUrl.url, containingFile, styleUrl.nodeForError, resourceType);
const resourceStr = this.resourceLoader.load(resourceUrl);

styles.push(resourceStr);
if (this.depTracker !== null) {
this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl));
try {
const resourceUrl = this.resourceLoader.resolve(styleUrl.url, containingFile);
const resourceStr = this.resourceLoader.load(resourceUrl);
styles.push(resourceStr);
if (this.depTracker !== null) {
this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl));
}
} catch {
if (diagnostics === undefined) {
diagnostics = [];
}
const resourceType =
styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ?
ResourceTypeForDiagnostics.StylesheetFromDecorator :
ResourceTypeForDiagnostics.StylesheetFromTemplate;
diagnostics.push(
this.makeResourceNotFoundError(styleUrl.url, styleUrl.nodeForError, resourceType)
.toDiagnostic());
}
}

Expand Down Expand Up @@ -496,8 +501,9 @@ export class ComponentDecoratorHandler implements
styles: styleResources,
template: templateResource,
},
isPoisoned: false,
isPoisoned,
},
diagnostics,
};
if (changeDetection !== null) {
output.analysis!.meta.changeDetection = changeDetection;
Expand Down Expand Up @@ -833,14 +839,14 @@ export class ComponentDecoratorHandler implements
let styles: string[] = [];
if (analysis.styleUrls !== null) {
for (const styleUrl of analysis.styleUrls) {
const resourceType =
styleUrl.source === ResourceTypeForDiagnostics.StylesheetFromDecorator ?
ResourceTypeForDiagnostics.StylesheetFromDecorator :
ResourceTypeForDiagnostics.StylesheetFromTemplate;
const resolvedStyleUrl = this._resolveResourceOrThrow(
styleUrl.url, containingFile, styleUrl.nodeForError, resourceType);
const styleText = this.resourceLoader.load(resolvedStyleUrl);
styles.push(styleText);
try {
const resolvedStyleUrl = this.resourceLoader.resolve(styleUrl.url, containingFile);
const styleText = this.resourceLoader.load(resolvedStyleUrl);
styles.push(styleText);
} catch (e) {
// Resource resolve failures should already be in the diagnostics list from the analyze
// stage. We do not need to do anything with them when updating resources.
}
}
}
if (analysis.inlineStyles !== null) {
Expand Down Expand Up @@ -978,10 +984,14 @@ export class ComponentDecoratorHandler implements
const styleUrlsExpr = component.get('styleUrls');
if (styleUrlsExpr !== undefined && ts.isArrayLiteralExpression(styleUrlsExpr)) {
for (const expression of stringLiteralElements(styleUrlsExpr)) {
const resourceUrl = this._resolveResourceOrThrow(
expression.text, containingFile, expression,
ResourceTypeForDiagnostics.StylesheetFromDecorator);
styles.add({path: absoluteFrom(resourceUrl), expression});
try {
const resourceUrl = this.resourceLoader.resolve(expression.text, containingFile);
styles.add({path: absoluteFrom(resourceUrl), expression});
} catch {
// Errors in style resource extraction do not need to be handled here. We will produce
// diagnostics for each one that fails in the analysis, after we evaluate the `styleUrls`
// expression to determine _all_ style resources, not just the string literals.
}
}
}

Expand All @@ -1006,22 +1016,27 @@ export class ComponentDecoratorHandler implements
throw createValueHasWrongTypeError(
templateUrlExpr, templateUrl, 'templateUrl must be a string');
}
const resourceUrl = this._resolveResourceOrThrow(
templateUrl, containingFile, templateUrlExpr, ResourceTypeForDiagnostics.Template);
const templatePromise =
this.resourceLoader.preload(resourceUrl, {type: 'template', containingFile});

// If the preload worked, then actually load and parse the template, and wait for any style
// URLs to resolve.
if (templatePromise !== undefined) {
return templatePromise.then(() => {
const templateDecl = this.parseTemplateDeclaration(decorator, component, containingFile);
const template = this.extractTemplate(node, templateDecl);
this.preanalyzeTemplateCache.set(node, template);
return template;
});
} else {
return Promise.resolve(null);
try {
const resourceUrl = this.resourceLoader.resolve(templateUrl, containingFile);
const templatePromise =
this.resourceLoader.preload(resourceUrl, {type: 'template', containingFile});

// If the preload worked, then actually load and parse the template, and wait for any style
// URLs to resolve.
if (templatePromise !== undefined) {
return templatePromise.then(() => {
const templateDecl =
this.parseTemplateDeclaration(decorator, component, containingFile);
const template = this.extractTemplate(node, templateDecl);
this.preanalyzeTemplateCache.set(node, template);
return template;
});
} else {
return Promise.resolve(null);
}
} catch (e) {
throw this.makeResourceNotFoundError(
templateUrl, templateUrlExpr, ResourceTypeForDiagnostics.Template);
}
} else {
const templateDecl = this.parseTemplateDeclaration(decorator, component, containingFile);
Expand Down Expand Up @@ -1186,18 +1201,21 @@ export class ComponentDecoratorHandler implements
throw createValueHasWrongTypeError(
templateUrlExpr, templateUrl, 'templateUrl must be a string');
}
const resourceUrl = this._resolveResourceOrThrow(
templateUrl, containingFile, templateUrlExpr, ResourceTypeForDiagnostics.Template);

return {
isInline: false,
interpolationConfig,
preserveWhitespaces,
templateUrl,
templateUrlExpression: templateUrlExpr,
resolvedTemplateUrl: resourceUrl,
sourceMapUrl: sourceMapUrl(resourceUrl),
};
try {
const resourceUrl = this.resourceLoader.resolve(templateUrl, containingFile);
return {
isInline: false,
interpolationConfig,
preserveWhitespaces,
templateUrl,
templateUrlExpression: templateUrlExpr,
resolvedTemplateUrl: resourceUrl,
sourceMapUrl: sourceMapUrl(resourceUrl),
};
} catch (e) {
throw this.makeResourceNotFoundError(
templateUrl, templateUrlExpr, ResourceTypeForDiagnostics.Template);
}
} else if (component.has('template')) {
return {
isInline: true,
Expand Down Expand Up @@ -1260,33 +1278,24 @@ export class ComponentDecoratorHandler implements
this.cycleAnalyzer.recordSyntheticImport(origin, imported);
}

/**
* Resolve the url of a resource relative to the file that contains the reference to it.
*
* Throws a FatalDiagnosticError when unable to resolve the file.
*/
private _resolveResourceOrThrow(
file: string, basePath: string, nodeForError: ts.Node,
resourceType: ResourceTypeForDiagnostics): string {
try {
return this.resourceLoader.resolve(file, basePath);
} catch (e) {
let errorText: string;
switch (resourceType) {
case ResourceTypeForDiagnostics.Template:
errorText = `Could not find template file '${file}'.`;
break;
case ResourceTypeForDiagnostics.StylesheetFromTemplate:
errorText = `Could not find stylesheet file '${file}' linked from the template.`;
break;
case ResourceTypeForDiagnostics.StylesheetFromDecorator:
errorText = `Could not find stylesheet file '${file}'.`;
break;
}

throw new FatalDiagnosticError(
ErrorCode.COMPONENT_RESOURCE_NOT_FOUND, nodeForError, errorText);
private makeResourceNotFoundError(
file: string, nodeForError: ts.Node,
resourceType: ResourceTypeForDiagnostics): FatalDiagnosticError {
let errorText: string;
switch (resourceType) {
case ResourceTypeForDiagnostics.Template:
errorText = `Could not find template file '${file}'.`;
break;
case ResourceTypeForDiagnostics.StylesheetFromTemplate:
errorText = `Could not find stylesheet file '${file}' linked from the template.`;
break;
case ResourceTypeForDiagnostics.StylesheetFromDecorator:
errorText = `Could not find stylesheet file '${file}'.`;
break;
}

return new FatalDiagnosticError(
ErrorCode.COMPONENT_RESOURCE_NOT_FOUND, nodeForError, errorText);
}

private _extractTemplateStyleUrls(template: ParsedTemplateWithSource): StyleUrlMeta[] {
Expand Down
34 changes: 33 additions & 1 deletion packages/language-service/ivy/test/quick_info_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,8 +553,40 @@ describe('quick info', () => {
expectQuickInfo(
{templateOverride, expectedSpanText: 'date', expectedDisplayString: '(pipe) DatePipe'});
});
});

it('should still get quick info if there is an invalid css resource', () => {
project = env.addProject('test', {
'app.ts': `
import {Component, NgModule} from '@angular/core';

@Component({
selector: 'some-cmp',
templateUrl: './app.html',
styleUrls: ['./does_not_exist'],
})
export class SomeCmp {
myValue!: string;
}

@NgModule({
declarations: [SomeCmp],
})
export class AppModule{
}
`,
'app.html': `{{myValue}}`,
});
const diagnostics = project.getDiagnosticsForFile('app.ts');
expect(diagnostics.length).toBe(1);
expect(diagnostics[0].messageText)
.toEqual(`Could not find stylesheet file './does_not_exist'.`);

const template = project.openFile('app.html');
template.moveCursorToText('{{myVa¦lue}}');
const quickInfo = template.getQuickInfoAtPosition();
expect(toText(quickInfo!.displayParts)).toEqual('(property) SomeCmp.myValue: string');
});
});

function expectQuickInfo(
{templateOverride, expectedSpanText, expectedDisplayString}:
Expand Down