refactor(schematics): do not glob for additional stylesheets multiple times (#13155)

devversion · mmalerba · commit 90ba6fb77ae0 · 2018-09-18T14:07:34.000-07:00
* Currently we glob the _whole project_ for additional stylesheets which couldn't be detected by the component walker. This glob runs for each source file multiple times (4 rules; X source files; 4 * x times run the glob). With this change, we only glob once at initialization.
diff --git a/src/lib/schematics/update/rules/attribute-selectors/attributeSelectorsStylesheetRule.ts b/src/lib/schematics/update/rules/attribute-selectors/attributeSelectorsStylesheetRule.ts
@@ -7,8 +7,8 @@
  */
 
 import {green, red} from 'chalk';
-import {sync as globSync} from 'glob';
 import {IOptions, Replacement, RuleFailure, Rules} from 'tslint';
+import * as ts from 'typescript';
 import {attributeSelectors} from '../../material/data/attribute-selectors';
 import {getChangesForTarget} from '../../material/transform-change-data';
 import {ExternalResource} from '../../tslint/component-file';
@@ -18,7 +18,6 @@ import {
   createExternalReplacementFailure,
 } from '../../tslint/rule-failures';
 import {findAllSubstringIndices} from '../../typescript/literal';
-import * as ts from 'typescript';
 
 /**
  * Rule that walks through every stylesheet in the application and updates outdated
@@ -37,11 +36,8 @@ export class Walker extends ComponentWalker {
   data = getChangesForTarget(this.getOptions()[0], attributeSelectors);
 
   constructor(sourceFile: ts.SourceFile, options: IOptions) {
-    // In some applications, developers will have global stylesheets that are not specified in any
-    // Angular component. Therefore we glob up all css and scss files outside of node_modules and
-    // dist and check them as well.
-    super(sourceFile, options, globSync('!(node_modules|dist)/**/*.+(css|scss)'));
-    this._reportExtraStylesheetFiles();
+    super(sourceFile, options);
+    this._reportExtraStylesheetFiles(options.ruleArguments[1]);
   }
 
   visitInlineStylesheet(literal: ts.StringLiteralLike) {
diff --git a/src/lib/schematics/update/rules/css-selectors/cssSelectorsStylesheetRule.ts b/src/lib/schematics/update/rules/css-selectors/cssSelectorsStylesheetRule.ts
@@ -7,7 +7,6 @@
  */
 
 import {green, red} from 'chalk';
-import {sync as globSync} from 'glob';
 import {IOptions, Replacement, RuleFailure, Rules} from 'tslint';
 import * as ts from 'typescript';
 import {cssSelectors} from '../../material/data/css-selectors';
@@ -36,11 +35,8 @@ export class Walker extends ComponentWalker {
   data = getChangesForTarget(this.getOptions()[0], cssSelectors);
 
   constructor(sourceFile: ts.SourceFile, options: IOptions) {
-    // In some applications, developers will have global stylesheets that are not specified in any
-    // Angular component. Therefore we glob up all css and scss files outside of node_modules and
-    // dist and check them as well.
-    super(sourceFile, options, globSync('!(node_modules|dist)/**/*.+(css|scss)'));
-    this._reportExtraStylesheetFiles();
+    super(sourceFile, options);
+    this._reportExtraStylesheetFiles(options.ruleArguments[1]);
   }
 
   visitInlineStylesheet(literal: ts.StringLiteralLike) {
diff --git a/src/lib/schematics/update/rules/element-selectors/elementSelectorsStylesheetRule.ts b/src/lib/schematics/update/rules/element-selectors/elementSelectorsStylesheetRule.ts
@@ -7,7 +7,6 @@
  */
 
 import {green, red} from 'chalk';
-import {sync as globSync} from 'glob';
 import {IOptions, Replacement, RuleFailure, Rules} from 'tslint';
 import * as ts from 'typescript';
 import {elementSelectors} from '../../material/data/element-selectors';
@@ -36,11 +35,8 @@ export class Walker extends ComponentWalker {
   data = getChangesForTarget(this.getOptions()[0], elementSelectors);
 
   constructor(sourceFile: ts.SourceFile, options: IOptions) {
-    // In some applications, developers will have global stylesheets that are not specified in any
-    // Angular component. Therefore we glob up all css and scss files outside of node_modules and
-    // dist and check them as well.
-    super(sourceFile, options, globSync('!(node_modules|dist)/**/*.+(css|scss)'));
-    this._reportExtraStylesheetFiles();
+    super(sourceFile, options);
+    this._reportExtraStylesheetFiles(options.ruleArguments[1]);
   }
 
   visitInlineStylesheet(literal: ts.StringLiteralLike) {
diff --git a/src/lib/schematics/update/rules/input-names/inputNamesStylesheetRule.ts b/src/lib/schematics/update/rules/input-names/inputNamesStylesheetRule.ts
@@ -7,7 +7,6 @@
  */
 
 import {green, red} from 'chalk';
-import {sync as globSync} from 'glob';
 import {IOptions, Replacement, RuleFailure, Rules} from 'tslint';
 import * as ts from 'typescript';
 import {inputNames} from '../../material/data/input-names';
@@ -41,11 +40,8 @@ export class Walker extends ComponentWalker {
   data = getChangesForTarget(this.getOptions()[0], inputNames);
 
   constructor(sourceFile: ts.SourceFile, options: IOptions) {
-    // In some applications, developers will have global stylesheets that are not specified in any
-    // Angular component. Therefore we glob up all css and scss files outside of node_modules and
-    // dist and check them as well.
-    super(sourceFile, options, globSync('!(node_modules|dist)/**/*.+(css|scss)'));
-    this._reportExtraStylesheetFiles();
+    super(sourceFile, options);
+    this._reportExtraStylesheetFiles(options.ruleArguments[1]);
   }
 
   visitInlineStylesheet(literal: ts.StringLiteralLike) {
diff --git a/src/lib/schematics/update/tslint-config.ts b/src/lib/schematics/update/tslint-config.ts
@@ -61,10 +61,13 @@ const rulesDirectory = globSync('rules/**/', {cwd: __dirname, absolute: true});
  * Creates a TSLint configuration object that can be passed to the schematic `TSLintFixTask`.
  * Each rule will have the specified target version as option which can be used to swap out
  * the upgrade data based on the given target version.
+ *
+ * Additionally we pass a list of additional style files to the TSLint rules because the component
+ * walker is not able to detect stylesheets which are not referenced by Angular.
  */
-export function createTslintConfig(target: TargetVersion) {
+export function createTslintConfig(target: TargetVersion, extraStyleFiles: string[]) {
   const rules = upgradeRules.reduce((result, ruleName) => {
-    result[ruleName] = [true, target];
+    result[ruleName] = [true, target, extraStyleFiles];
     return result;
   }, {});
 
diff --git a/src/lib/schematics/update/tslint/component-walker.spec.ts b/src/lib/schematics/update/tslint/component-walker.spec.ts
@@ -118,6 +118,44 @@ describe('ComponentWalker', () => {
     expect(() => walker.walk(sourceFile)).not.toThrow();
     expect(walker.visitInlineStylesheet).toHaveBeenCalledTimes(2);
   });
+
+  it('should be able to report additional stylesheet files', () => {
+    const sourceFile = createSourceFile(``);
+    const walker = new ComponentWalker(sourceFile, defaultRuleOptions);
+    const stylesheetPath = './shared-styles/global.css';
+
+    spyOn(walker, 'visitExternalStylesheet').and.callFake(node => {
+      expect(node.getFullText()).toBe('external');
+    });
+
+    mockFs({[stylesheetPath]: 'external'});
+
+    walker._reportExtraStylesheetFiles([stylesheetPath]);
+
+    expect(walker.visitExternalStylesheet).toHaveBeenCalledTimes(1);
+  });
+
+  it('should not visit stylesheet files multiple times', () => {
+    const sourceFile = createSourceFile(`
+      @Component({
+        styleUrls: ['./my-component.css']
+      })
+      export class MyComponent {}
+    `);
+    const walker = new ComponentWalker(sourceFile, defaultRuleOptions);
+    const stylePath = join(dirname(sourceFile.fileName), 'my-component.css');
+
+    spyOn(walker, 'visitExternalStylesheet').and.callFake(node => {
+      expect(node.getFullText()).toBe('external');
+    });
+
+    mockFs({[stylePath]: 'external'});
+
+    walker._reportExtraStylesheetFiles([stylePath]);
+    walker.walk(sourceFile);
+
+    expect(walker.visitExternalStylesheet).toHaveBeenCalledTimes(1);
+  });
 });
 
 /** TypeScript source file content that includes a component with inline styles. */
diff --git a/src/lib/schematics/update/tslint/component-walker.ts b/src/lib/schematics/update/tslint/component-walker.ts
@@ -6,12 +6,9 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-/**
- * TSLint custom walker implementation that also visits external and inline templates.
- */
 import {existsSync, readFileSync} from 'fs';
-import {dirname, join, resolve} from 'path';
-import {IOptions, RuleWalker} from 'tslint';
+import {dirname, resolve} from 'path';
+import {RuleWalker} from 'tslint';
 import * as ts from 'typescript';
 import {createComponentFile, ExternalResource} from './component-file';
 
@@ -27,13 +24,12 @@ export class ComponentWalker extends RuleWalker {
   visitExternalTemplate(_template: ExternalResource) {}
   visitExternalStylesheet(_stylesheet: ExternalResource) {}
 
-  private extraFiles: Set<string>;
-
-  constructor(sourceFile: ts.SourceFile, options: IOptions, extraFiles: string[] = []) {
-    super(sourceFile, options);
-
-    this.extraFiles = new Set(extraFiles.map(p => resolve(p)));
-  }
+  /**
+   * We keep track of all visited stylesheet files because we allow manually reporting external
+   * stylesheets which couldn't be detected by the component walker. Reporting these files multiple
+   * times will result in duplicated TSLint failures and replacements.
+   */
+  private _visitedStylesheetFiles: Set<string> = new Set<string>();
 
   visitNode(node: ts.Node) {
     if (node.kind === ts.SyntaxKind.CallExpression) {
@@ -83,12 +79,7 @@ export class ComponentWalker extends RuleWalker {
   }
 
   private _reportExternalTemplate(node: ts.StringLiteralLike) {
-    const templatePath = resolve(join(dirname(this.getSourceFile().fileName), node.text));
-
-    // Do not report the specified additional files multiple times.
-    if (this.extraFiles.has(templatePath)) {
-      return;
-    }
+    const templatePath = resolve(dirname(this.getSourceFile().fileName), node.text);
 
     // Check if the external template file exists before proceeding.
     if (!existsSync(templatePath)) {
@@ -113,12 +104,7 @@ export class ComponentWalker extends RuleWalker {
   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;
-        }
+        const stylePath = resolve(dirname(this.getSourceFile().fileName), node.text);
 
         // Check if the external stylesheet file exists before proceeding.
         if (!existsSync(stylePath)) {
@@ -130,20 +116,22 @@ export class ComponentWalker extends RuleWalker {
     });
   }
 
-  _reportExternalStyle(stylePath: string) {
+  private _reportExternalStyle(stylePath: string) {
+    // Keep track of all reported external stylesheets because we allow reporting additional
+    // stylesheet files which couldn't be detected by the component walker. This allows us to
+    // ensure that no stylesheet files are visited multiple times.
+    if (this._visitedStylesheetFiles.has(stylePath)) {
+      return;
+    }
+
+    this._visitedStylesheetFiles.add(stylePath);
+
     // Create a fake TypeScript source file that includes the stylesheet content.
     const stylesheetFile = createComponentFile(stylePath, readFileSync(stylePath, 'utf8'));
 
     this.visitExternalStylesheet(stylesheetFile);
   }
 
-  /** Reports all extra files that have been specified at initialization. */
-  // TODO(devversion): this should be done automatically but deferred because
-  // the base class "data" property member is not ready at initialization.
-  _reportExtraStylesheetFiles() {
-    this.extraFiles.forEach(file => this._reportExternalStyle(file));
-  }
-
   /**
    * Recursively searches for the metadata object literal expression inside of a directive call
    * expression. Since expression calls can be nested through *parenthesized* expressions, we
@@ -169,4 +157,9 @@ export class ComponentWalker extends RuleWalker {
     this.addFailureAtNode(node, `Could not resolve resource file: "${resourceUrl}". ` +
         `Skipping automatic upgrade for this file.`);
   }
+
+  /** Reports the specified additional stylesheets. */
+  _reportExtraStylesheetFiles(filePaths: string[]) {
+    filePaths.forEach(filePath => this._reportExternalStyle(resolve(filePath)));
+  }
 }
diff --git a/src/lib/schematics/update/update.ts b/src/lib/schematics/update/update.ts
@@ -8,9 +8,10 @@
 
 import {Rule, SchematicContext, TaskId, Tree} from '@angular-devkit/schematics';
 import {RunSchematicTask, TslintFixTask} from '@angular-devkit/schematics/tasks';
+import {sync as globSync} from 'glob';
 import {TargetVersion} from './index';
 import {getProjectTsConfigPaths} from './project-tsconfig-paths';
-import {createTslintConfig} from './tslint-update';
+import {createTslintConfig} from './tslint-config';
 
 /** Entry point for `ng update` from Angular CLI. */
 export function createUpdateRule(targetVersion: TargetVersion): Rule {
@@ -23,8 +24,11 @@ export function createUpdateRule(targetVersion: TargetVersion): Rule {
       throw new Error('Could not find any tsconfig file. Please submit an issue on the Angular ' +
         'Material repository that includes the name of your TypeScript configuration.');
     }
-
-    const tslintConfig = createTslintConfig(targetVersion);
+    // In some applications, developers will have global stylesheets which are not specified in any
+    // Angular component. Therefore we glob up all CSS and SCSS files outside of node_modules and
+    // dist. The files will be read by the individual stylesheet rules and checked.
+    const extraStyleFiles = globSync('!(node_modules|dist)/**/*.+(css|scss)', {absolute: true});
+    const tslintConfig = createTslintConfig(targetVersion, extraStyleFiles);
 
     for (const tsconfig of projectTsConfigPaths) {
       // Run the update tslint rules.
