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

devversion · jelbourn · commit b30cf6e7ee6c · 2018-09-16T08:10:11.000-07:00
Currently we just treat every value of `@Component#template` or `@Component#styles` as `ts.StringLiteral`. This works in the most cases, but there can be also developers that inline their template using some specific Webpack loaders or variables.
diff --git a/src/lib/schematics/update/rules/attribute-selectors/attributeSelectorsStylesheetRule.ts b/src/lib/schematics/update/rules/attribute-selectors/attributeSelectorsStylesheetRule.ts
@@ -44,7 +44,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));
   }
diff --git a/src/lib/schematics/update/rules/attribute-selectors/attributeSelectorsTemplateRule.ts b/src/lib/schematics/update/rules/attribute-selectors/attributeSelectorsTemplateRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/checkTemplateMiscRule.ts b/src/lib/schematics/update/rules/checkTemplateMiscRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/css-selectors/cssSelectorsStylesheetRule.ts b/src/lib/schematics/update/rules/css-selectors/cssSelectorsStylesheetRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/css-selectors/cssSelectorsTemplateRule.ts b/src/lib/schematics/update/rules/css-selectors/cssSelectorsTemplateRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/element-selectors/elementSelectorsStylesheetRule.ts b/src/lib/schematics/update/rules/element-selectors/elementSelectorsStylesheetRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/element-selectors/elementSelectorsTemplateRule.ts b/src/lib/schematics/update/rules/element-selectors/elementSelectorsTemplateRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/input-names/inputNamesStylesheetRule.ts b/src/lib/schematics/update/rules/input-names/inputNamesStylesheetRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/input-names/inputNamesTemplateRule.ts b/src/lib/schematics/update/rules/input-names/inputNamesTemplateRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/rules/output-names/outputNamesTemplateRule.ts b/src/lib/schematics/update/rules/output-names/outputNamesTemplateRule.ts
@@ -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));
   }
diff --git a/src/lib/schematics/update/tslint/component-walker.spec.ts b/src/lib/schematics/update/tslint/component-walker.spec.ts
@@ -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', () => {
@@ -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);
   });
 });
 
diff --git a/src/lib/schematics/update/tslint/component-walker.ts b/src/lib/schematics/update/tslint/component-walker.ts
@@ -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) {}
@@ -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.
@@ -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;
     }
 
@@ -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'));
 
@@ -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.`);
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ export class Walker extends ComponentWalker {`
`44`	`44`	`this._reportExtraStylesheetFiles();`
`45`	`45`	`}`
`46`	`46`
`47`		`- visitInlineStylesheet(literal: ts.StringLiteral) {`
	`47`	`+ visitInlineStylesheet(literal: ts.StringLiteralLike) {`
`48`	`48`	`this._createReplacementsForContent(literal, literal.getText())`
`49`	`49`	`.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));`
`50`	`50`	`}`
Original file line number	Diff line number	Diff line change
`@@ -34,7 +34,7 @@ export class Walker extends ComponentWalker {`
`34`	`34`	`/** Change data that upgrades to the specified target version. */`
`35`	`35`	`data = getChangesForTarget(this.getOptions()[0], attributeSelectors);`
`36`	`36`
`37`		`- visitInlineTemplate(template: ts.StringLiteral) {`
	`37`	`+ visitInlineTemplate(template: ts.StringLiteralLike) {`
`38`	`38`	`this._createReplacementsForContent(template, template.getText())`
`39`	`39`	`.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));`
`40`	`40`	`}`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ export class Rule extends Rules.AbstractRule {`
`26`	`26`
`27`	`27`	`export class Walker extends ComponentWalker {`
`28`	`28`
`29`		`- visitInlineTemplate(template: ts.StringLiteral) {`
	`29`	`+ visitInlineTemplate(template: ts.StringLiteralLike) {`
`30`	`30`	`this._createFailuresForContent(template, template.getText())`
`31`	`31`	`.forEach(data => this.addFailureFromStartToEnd(data.start, data.end, data.message));`
`32`	`32`	`}`
Original file line number	Diff line number	Diff line change
`@@ -43,7 +43,7 @@ export class Walker extends ComponentWalker {`
`43`	`43`	`this._reportExtraStylesheetFiles();`
`44`	`44`	`}`
`45`	`45`
`46`		`- visitInlineStylesheet(literal: ts.StringLiteral) {`
	`46`	`+ visitInlineStylesheet(literal: ts.StringLiteralLike) {`
`47`	`47`	`this._createReplacementsForContent(literal, literal.getText())`
`48`	`48`	`.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));`
`49`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -48,7 +48,7 @@ export class Walker extends ComponentWalker {`
`48`	`48`	`this._reportExtraStylesheetFiles();`
`49`	`49`	`}`
`50`	`50`
`51`		`- visitInlineStylesheet(literal: ts.StringLiteral) {`
	`51`	`+ visitInlineStylesheet(literal: ts.StringLiteralLike) {`
`52`	`52`	`this._createReplacementsForContent(literal, literal.getText())`
`53`	`53`	`.forEach(data => addFailureAtReplacement(this, data.failureMessage, data.replacement));`
`54`	`54`	`}`