Skip to content

refactor(schematics): gracefully handle non statically analyzable values #13143

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles();
}

visitInlineStylesheet(literal: ts.StringLiteral) {
visitInlineStylesheet(literal: ts.StringLiteralLike) {
Copy link
Member

Choose a reason for hiding this comment

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

How did you know about this type? I tried to find something for it, but the TS compiler looks mostly undocumented.

Copy link
Member Author

Choose a reason for hiding this comment

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

I most of the time just look at the source or at the typescript.d.ts file. The d.ts files are pretty good to quickly explore the public API.

this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], attributeSelectors);

visitInlineTemplate(template: ts.StringLiteral) {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
2 changes: 1 addition & 1 deletion src/lib/schematics/update/rules/checkTemplateMiscRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export class Rule extends Rules.AbstractRule {

export class Walker extends ComponentWalker {

visitInlineTemplate(template: ts.StringLiteral) {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createFailuresForContent(template, template.getText())
.forEach(data => this.addFailureFromStartToEnd(data.start, data.end, data.message));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles();
}

visitInlineStylesheet(literal: ts.StringLiteral) {
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], cssSelectors);

visitInlineTemplate(template: ts.StringLiteral) {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles();
}

visitInlineStylesheet(literal: ts.StringLiteral) {
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], elementSelectors);

visitInlineTemplate(template: ts.StringLiteral) {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class Walker extends ComponentWalker {
this._reportExtraStylesheetFiles();
}

visitInlineStylesheet(literal: ts.StringLiteral) {
visitInlineStylesheet(literal: ts.StringLiteralLike) {
this._createReplacementsForContent(literal, literal.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], inputNames);

visitInlineTemplate(template: ts.StringLiteral) {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class Walker extends ComponentWalker {
/** Change data that upgrades to the specified target version. */
data = getChangesForTarget(this.getOptions()[0], outputNames);

visitInlineTemplate(template: ts.StringLiteral) {
visitInlineTemplate(template: ts.StringLiteralLike) {
this._createReplacementsForContent(template, template.getText())
.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));
}
Expand Down
33 changes: 28 additions & 5 deletions src/lib/schematics/update/tslint/component-walker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,8 @@ describe('ComponentWalker', () => {
const sourceFile = createSourceFile(externalStylesheetSource);
const walker = new ComponentWalker(sourceFile, defaultRuleOptions);

spyOn(console, 'error');

expect(() => walker.walk(sourceFile)).not.toThrow();
expect(console.error).toHaveBeenCalledTimes(1);
expect(walker.getFailures().length).toBe(1);
});

it('should report inline templates', () => {
Expand Down Expand Up @@ -90,10 +88,35 @@ describe('ComponentWalker', () => {
const sourceFile = createSourceFile(externalTemplateSource);
const walker = new ComponentWalker(sourceFile, defaultRuleOptions);

spyOn(console, 'error');
expect(() => walker.walk(sourceFile)).not.toThrow();
expect(walker.getFailures().length).toBe(1);
});

it('should not throw if the inline template could not be resolved', () => {
const sourceFile = createSourceFile(`
@Component({
template: myTemplate,
})
export class MyComponent {}
`);
const walker = new ComponentWalker(sourceFile, defaultRuleOptions);

expect(() => walker.walk(sourceFile)).not.toThrow();
});

it('should not throw if inline styles could not be resolved', () => {
const sourceFile = createSourceFile(`
@Component({
styles: [styleA, styleB, 'c', 'd'],
})
export class MyComponent {}
`);
const walker = new ComponentWalker(sourceFile, defaultRuleOptions);

spyOn(walker, 'visitInlineStylesheet');

expect(() => walker.walk(sourceFile)).not.toThrow();
expect(console.error).toHaveBeenCalledTimes(1);
expect(walker.visitInlineStylesheet).toHaveBeenCalledTimes(2);
});
});

Expand Down
88 changes: 49 additions & 39 deletions src/lib/schematics/update/tslint/component-walker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import {createComponentFile, ExternalResource} from './component-file';
*/
export class ComponentWalker extends RuleWalker {

visitInlineTemplate(_template: ts.StringLiteral) {}
visitInlineStylesheet(_stylesheet: ts.StringLiteral) {}
visitInlineTemplate(_template: ts.StringLiteralLike) {}
visitInlineStylesheet(_stylesheet: ts.StringLiteralLike) {}

visitExternalTemplate(_template: ExternalResource) {}
visitExternalStylesheet(_stylesheet: ExternalResource) {}
Expand Down Expand Up @@ -63,45 +63,26 @@ export class ComponentWalker extends RuleWalker {

for (const property of directiveMetadata.properties as ts.NodeArray<ts.PropertyAssignment>) {
const propertyName = property.name.getText();
const initializerKind = property.initializer.kind;

if (propertyName === 'template') {
this.visitInlineTemplate(property.initializer as ts.StringLiteral);
if (propertyName === 'template' && ts.isStringLiteralLike(property.initializer)) {
this.visitInlineTemplate(property.initializer);
}

if (propertyName === 'templateUrl' && initializerKind === ts.SyntaxKind.StringLiteral) {
this._reportExternalTemplate(property.initializer as ts.StringLiteral);
if (propertyName === 'templateUrl' && ts.isStringLiteralLike(property.initializer)) {
this._reportExternalTemplate(property.initializer);
}

if (propertyName === 'styles' && initializerKind === ts.SyntaxKind.ArrayLiteralExpression) {
this._reportInlineStyles(property.initializer as ts.ArrayLiteralExpression);
if (propertyName === 'styles' && ts.isArrayLiteralExpression(property.initializer)) {
this._reportInlineStyles(property.initializer);
}

if (propertyName === 'styleUrls' &&
initializerKind === ts.SyntaxKind.ArrayLiteralExpression) {
this._visitExternalStylesArrayLiteral(property.initializer as ts.ArrayLiteralExpression);
if (propertyName === 'styleUrls' && ts.isArrayLiteralExpression(property.initializer)) {
this._visitExternalStylesArrayLiteral(property.initializer);
}
}
}

private _reportInlineStyles(inlineStyles: ts.ArrayLiteralExpression) {
inlineStyles.elements.forEach(element => {
this.visitInlineStylesheet(element as ts.StringLiteral);
});
}

private _visitExternalStylesArrayLiteral(styleUrls: ts.ArrayLiteralExpression) {
styleUrls.elements.forEach((node: ts.StringLiteral) => {
const stylePath = resolve(join(dirname(this.getSourceFile().fileName), node.text));

// Do not report the specified additional files multiple times.
if (!this.extraFiles.has(stylePath)) {
this._reportExternalStyle(stylePath);
}
});
}

private _reportExternalTemplate(node: ts.StringLiteral) {
private _reportExternalTemplate(node: ts.StringLiteralLike) {
const templatePath = resolve(join(dirname(this.getSourceFile().fileName), node.text));

// Do not report the specified additional files multiple times.
Expand All @@ -111,8 +92,7 @@ export class ComponentWalker extends RuleWalker {

// Check if the external template file exists before proceeding.
if (!existsSync(templatePath)) {
console.error(`PARSE ERROR: ${this.getSourceFile().fileName}:` +
` Could not find template: "${templatePath}".`);
this._createResourceNotFoundFailure(node, templatePath);
return;
}

Expand All @@ -122,14 +102,35 @@ export class ComponentWalker extends RuleWalker {
this.visitExternalTemplate(templateFile);
}

_reportExternalStyle(stylePath: string) {
// Check if the external stylesheet file exists before proceeding.
if (!existsSync(stylePath)) {
console.error(`PARSE ERROR: ${this.getSourceFile().fileName}: ` +
`Could not find stylesheet: "${stylePath}".`);
return;
}
private _reportInlineStyles(expression: ts.ArrayLiteralExpression) {
expression.elements.forEach(node => {
if (ts.isStringLiteralLike(node)) {
this.visitInlineStylesheet(node);
}
});
}

private _visitExternalStylesArrayLiteral(expression: ts.ArrayLiteralExpression) {
expression.elements.forEach(node => {
if (ts.isStringLiteralLike(node)) {
const stylePath = resolve(join(dirname(this.getSourceFile().fileName), node.text));

// Do not report the specified additional files multiple times.
if (this.extraFiles.has(stylePath)) {
return;
}

// Check if the external stylesheet file exists before proceeding.
if (!existsSync(stylePath)) {
return this._createResourceNotFoundFailure(node, stylePath);
}

this._reportExternalStyle(stylePath);
}
});
}

_reportExternalStyle(stylePath: string) {
// Create a fake TypeScript source file that includes the stylesheet content.
const stylesheetFile = createComponentFile(stylePath, readFileSync(stylePath, 'utf8'));

Expand Down Expand Up @@ -159,4 +160,13 @@ export class ComponentWalker extends RuleWalker {

return null;
}

/**
* Creates a TSLint failure that reports that the resource file that belongs to the specified
* TypeScript node could not be resolved in the file system.
*/
private _createResourceNotFoundFailure(node: ts.Node, resourceUrl: string) {
this.addFailureAtNode(node, `Could not resolve resource file: "${resourceUrl}". ` +
`Skipping automatic upgrade for this file.`);
}
}