fix(material/schematics): theming migration schematic not inserting use statement at the top of the file in some cases

Our logic for determining where to insert an `@use` statement is to look for the first `@import` and insert before it. The problem is that if the theming import is the only one in the file and it's further down, we'll insert it incorrectly, because `@use` has to always be above other statements.

Author: crisbeto

src/material/schematics/ng-update/migrations/theming-api-v12/migration.ts

@@ -16,6 +16,12 @@ import {
   unprefixedRemovedVariables
 } from './config';
 
+/** The result of a search for imports and namespaces in a file. */
+interface DetectImportResult {
+  imports: string[];
+  namespaces: string[];
+}
+
 /**
  * Migrates the content of a file to the new theming API. Note that this migration is using plain
  * string manipulation, rather than the AST from PostCSS and the schematics string manipulation
@@ -40,8 +46,8 @@ export function migrateFileContent(content: string,
 
   // Try to migrate the symbols even if there are no imports. This is used
   // to cover the case where the Components symbols were used transitively.
-  content = migrateMaterialSymbols(content, newMaterialImportPath, materialResults.namespaces);
-  content = migrateCdkSymbols(content, newCdkImportPath, cdkResults.namespaces);
+  content = migrateMaterialSymbols(content, newMaterialImportPath, materialResults);
+  content = migrateCdkSymbols(content, newCdkImportPath, cdkResults);
   content = replaceRemovedVariables(content, removedMaterialVariables);
 
   // We can assume that the migration has taken care of any Components symbols that were
@@ -64,7 +70,7 @@ export function migrateFileContent(content: string,
  * @param content File content in which to look for imports.
  * @param prefix Prefix that the imports should start with.
  */
-function detectImports(content: string, prefix: string): {imports: string[], namespaces: string[]} {
+function detectImports(content: string, prefix: string): DetectImportResult {
   if (prefix[prefix.length - 1] !== '/') {
     // Some of the logic further down makes assumptions about the import depth.
     throw Error(`Prefix "${prefix}" has to end in a slash.`);
@@ -96,47 +102,49 @@ function detectImports(content: string, prefix: string): {imports: string[], nam
 }
 
 /** Migrates the Material symbls in a file. */
-function migrateMaterialSymbols(content: string, importPath: string, namespaces: string[]): string {
+function migrateMaterialSymbols(content: string, importPath: string,
+                                detectedImports: DetectImportResult): string {
   const initialContent = content;
   const namespace = 'mat';
 
   // Migrate the mixins.
-  content = renameSymbols(content, materialMixins, namespaces, mixinKeyFormatter,
+  content = renameSymbols(content, materialMixins, detectedImports.namespaces, mixinKeyFormatter,
     getMixinValueFormatter(namespace));
 
   // Migrate the functions.
-  content = renameSymbols(content, materialFunctions, namespaces, functionKeyFormatter,
-    getFunctionValueFormatter(namespace));
+  content = renameSymbols(content, materialFunctions, detectedImports.namespaces,
+    functionKeyFormatter, getFunctionValueFormatter(namespace));
 
   // Migrate the variables.
-  content = renameSymbols(content, materialVariables, namespaces, variableKeyFormatter,
-    getVariableValueFormatter(namespace));
+  content = renameSymbols(content, materialVariables, detectedImports.namespaces,
+    variableKeyFormatter, getVariableValueFormatter(namespace));
 
   if (content !== initialContent) {
     // Add an import to the new API only if any of the APIs were being used.
-    content = insertUseStatement(content, importPath, namespace);
+    content = insertUseStatement(content, importPath, detectedImports.imports, namespace);
   }
 
   return content;
 }
 
 /** Migrates the CDK symbols in a file. */
-function migrateCdkSymbols(content: string, importPath: string, namespaces: string[]): string {
+function migrateCdkSymbols(content: string, importPath: string,
+                           detectedImports: DetectImportResult): string {
   const initialContent = content;
   const namespace = 'cdk';
 
   // Migrate the mixins.
-  content = renameSymbols(content, cdkMixins, namespaces, mixinKeyFormatter,
+  content = renameSymbols(content, cdkMixins, detectedImports.namespaces, mixinKeyFormatter,
     getMixinValueFormatter(namespace));
 
   // Migrate the variables.
-  content = renameSymbols(content, cdkVariables, namespaces, variableKeyFormatter,
+  content = renameSymbols(content, cdkVariables, detectedImports.namespaces, variableKeyFormatter,
     getVariableValueFormatter(namespace));
 
   // Previously the CDK symbols were exposed through `material/theming`, but now we have a
   // dedicated entrypoint for the CDK. Only add an import for it if any of the symbols are used.
   if (content !== initialContent) {
-    content = insertUseStatement(content, importPath, namespace);
+    content = insertUseStatement(content, importPath, detectedImports.imports, namespace);
   }
 
   return content;
@@ -175,11 +183,21 @@ function renameSymbols(content: string,
 }
 
 /** Inserts an `@use` statement in a string. */
-function insertUseStatement(content: string, importPath: string, namespace: string): string {
+function insertUseStatement(content: string, importPath: string, importsToIgnore: string[],
+                            namespace: string): string {
+  // We want to find the first import that isn't in the list of ignored imports or find nothing,
+  // because the imports being replaced might be the only ones in the file and they can be further
+  // down. An easy way to do this is to replace the imports with a random character and run
+  // `indexOf` on the result. This isn't the most efficient way of doing it, but it's more compact
+  // and it allows us to easily deal with things like comment nodes.
+  const contentToSearch = importsToIgnore.reduce((accumulator, current) =>
+    accumulator.replace(current, '◬'.repeat(current.length)), content);
+
   // Sass has a limitation that all `@use` declarations have to come before `@import` so we have
   // to find the first import and insert before it. Technically we can get away with always
   // inserting at 0, but the file may start with something like a license header.
-  const newImportIndex = Math.max(0, content.indexOf('@import '));
+  const newImportIndex = Math.max(0, contentToSearch.indexOf('@import '));
+
   return content.slice(0, newImportIndex) + `@use '${importPath}' as ${namespace};\n` +
          content.slice(newImportIndex);
 }

src/material/schematics/ng-update/test-cases/v12/misc/theming-api-v12.spec.ts

@@ -423,8 +423,8 @@ describe('v12 theming API migration', () => {
     await runMigration();
 
     expect(splitFile(THEME_PATH)).toEqual([
-      `@use '~@angular/material' as mat;`,
       `@use '~@angular/cdk' as cdk;`,
+      `@use '~@angular/material' as mat;`,
 
       `@include cdk.overlay();`,
 
@@ -659,4 +659,38 @@ describe('v12 theming API migration', () => {
     ]);
   });
 
+  it('should insert the @use statement at the top of the file, if the theming import is ' +
+     'the only import in the file and there is other content before it', async () => {
+    writeLines(THEME_PATH, [
+      `:host {`,
+        `display: block;`,
+        `width: 100%;`,
+      `}`,
+      ``,
+      `@import '~@angular/material/theming';`,
+      ``,
+      `.button {`,
+        `@include mat-elevation(4);`,
+        `padding: 8px;`,
+      `}`,
+    ]);
+
+    await runMigration();
+
+    expect(splitFile(THEME_PATH)).toEqual([
+      `@use '~@angular/material' as mat;`,
+      `:host {`,
+        `display: block;`,
+        `width: 100%;`,
+      `}`,
+      ``,
+      ``,
+      `.button {`,
+        `@include mat.elevation(4);`,
+        `padding: 8px;`,
+      `}`,
+    ]);
+  });
+
+
 });