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.

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

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
71 changes: 52 additions & 19 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
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 @@ -265,9 +265,15 @@ export class ComponentDecoratorHandler implements
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});
try {
const resourceUrl =
this._resolveResourceOrThrow(styleUrl, containingFile, nodeForError, resourceType);
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.
Expand Down Expand Up @@ -326,6 +332,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 @@ -411,13 +419,26 @@ export class ComponentDecoratorHandler implements
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);
try {
const resourceUrl = this._resolveResourceOrThrow(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than this try/catch, we should make _resolveResourceOrThrow not throw, and instead return some kind of error that we can choose to deal with (or ignore) as we see fit. Possibly this means its return value will be some kind of union between a success type and a failure type that also includes a diagnostic.

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));
styles.push(resourceStr);

if (this.depTracker !== null) {
this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl));
}
} catch (e) {
if (e instanceof FatalDiagnosticError) {
if (diagnostics === undefined) {
diagnostics = [];
}
diagnostics.push(makeDiagnostic(e.code, e.node, e.message, e.relatedInformation));
isPoisoned = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPoisoned means "cannot be relied upon by consuming components" - a component shouldn't be poisoned because its own template or stylesheets are unavailable as they're not a part of its public API.

} else {
throw e;
}
}
}

Expand Down Expand Up @@ -496,8 +517,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 @@ -837,10 +859,15 @@ export class ComponentDecoratorHandler implements
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._resolveResourceOrThrow(
styleUrl.url, containingFile, styleUrl.nodeForError, resourceType);
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 +1005,16 @@ 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._resolveResourceOrThrow(
expression.text, containingFile, expression,
ResourceTypeForDiagnostics.StylesheetFromDecorator);
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 Down
34 changes: 33 additions & 1 deletion packages/language-service/ivy/test/quick_info_spec.ts
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