fix(material/schematics): don't migrate commented code in theming API migration (#23004)

Adds some logic that escapes comments so that the theming API migration doesn't pick them up.

Author: crisbeto

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

@@ -29,11 +29,20 @@ interface ExtraSymbols {
   variables?: Record<string, string>;
 }
 
+/** Possible pairs of comment characters in a Sass file. */
+const commentPairs = new Map<string, string>([['/*', '*/'], ['//', '\n']]);
+
+/** Prefix for the placeholder that will be used to escape comments. */
+const commentPlaceholderStart = '__<<ngThemingMigrationEscapedComment';
+
+/** Suffix for the comment escape placeholder. */
+const commentPlaceholderEnd = '>>__';
+
 /**
  * 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
  * APIs, because it allows us to run it inside g3 and to avoid introducing new dependencies.
- * @param content Content of the file.
+ * @param fileContent Content of the file.
  * @param oldMaterialPrefix Prefix with which the old Material imports should start.
  *   Has to end with a slash. E.g. if `@import '~@angular/material/theming'` should be
  *   matched, the prefix would be `~@angular/material/`.
@@ -44,21 +53,22 @@ interface ExtraSymbols {
  * @param newCdkImportPath New import to the CDK Sass APIs (e.g. `~@angular/cdk`).
  * @param excludedImports Pattern that can be used to exclude imports from being processed.
  */
-export function migrateFileContent(content: string,
+export function migrateFileContent(fileContent: string,
                                    oldMaterialPrefix: string,
                                    oldCdkPrefix: string,
                                    newMaterialImportPath: string,
                                    newCdkImportPath: string,
                                    extraMaterialSymbols: ExtraSymbols = {},
                                    excludedImports?: RegExp): string {
+  let {content, placeholders} = escapeComments(fileContent);
   const materialResults = detectImports(content, oldMaterialPrefix, excludedImports);
   const cdkResults = detectImports(content, oldCdkPrefix, excludedImports);
 
   // 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 = migrateCdkSymbols(content, newCdkImportPath, cdkResults);
+  content = migrateCdkSymbols(content, newCdkImportPath, placeholders, cdkResults);
   content = migrateMaterialSymbols(
-      content, newMaterialImportPath, materialResults, extraMaterialSymbols);
+      content, newMaterialImportPath, materialResults, placeholders, extraMaterialSymbols);
   content = replaceRemovedVariables(content, removedMaterialVariables);
 
   // We can assume that the migration has taken care of any Components symbols that were
@@ -73,7 +83,7 @@ export function migrateFileContent(content: string,
     content = removeStrings(content, cdkResults.imports);
   }
 
-  return content;
+  return restoreComments(content, placeholders);
 }
 
 /**
@@ -121,6 +131,7 @@ function detectImports(content: string, prefix: string,
 /** Migrates the Material symbols in a file. */
 function migrateMaterialSymbols(content: string, importPath: string,
                                 detectedImports: DetectImportResult,
+                                commentPlaceholders: Record<string, string>,
                                 extraMaterialSymbols: ExtraSymbols = {}): string {
   const initialContent = content;
   const namespace = 'mat';
@@ -142,14 +153,15 @@ function migrateMaterialSymbols(content: string, importPath: string,
 
   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, namespace, commentPlaceholders);
   }
 
   return content;
 }
 
 /** Migrates the CDK symbols in a file. */
 function migrateCdkSymbols(content: string, importPath: string,
+                           commentPlaceholders: Record<string, string>,
                            detectedImports: DetectImportResult): string {
   const initialContent = content;
   const namespace = 'cdk';
@@ -165,7 +177,7 @@ function migrateCdkSymbols(content: string, importPath: string,
   // 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, namespace, commentPlaceholders);
   }
 
   return content;
@@ -203,7 +215,8 @@ 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, namespace: string,
+                            commentPlaceholders: Record<string, string>): string {
   // If the content already has the `@use` import, we don't need to add anything.
   if (new RegExp(`@use +['"]${importPath}['"]`, 'g').test(content)) {
     return content;
@@ -217,9 +230,15 @@ function insertUseStatement(content: string, importPath: string, namespace: stri
   let newImportIndex = 0;
 
   // One special case is if the file starts with a license header which we want to preserve on top.
-  if (content.trim().startsWith('/*')) {
-    const commentEndIndex = content.indexOf('*/', content.indexOf('/*'));
-    newImportIndex = content.indexOf('\n', commentEndIndex) + 1;
+  if (content.trim().startsWith(commentPlaceholderStart)) {
+    const commentStartIndex = content.indexOf(commentPlaceholderStart);
+    newImportIndex = content.indexOf(commentPlaceholderEnd, commentStartIndex + 1) +
+        commentPlaceholderEnd.length;
+    // If the leading comment doesn't end with a newline,
+    // we need to insert the import at the next line.
+    if (!commentPlaceholders[content.slice(commentStartIndex, newImportIndex)].endsWith('\n')) {
+      newImportIndex = Math.max(newImportIndex, content.indexOf('\n', newImportIndex) + 1);
+    }
   }
 
   return content.slice(0, newImportIndex) + `@use '${importPath}' as ${namespace};\n` +
@@ -327,3 +346,48 @@ function replaceRemovedVariables(content: string, variables: Record<string, stri
 
   return content;
 }
+
+
+/**
+ * Replaces all of the comments in a Sass file with placeholders and
+ * returns the list of placeholders so they can be restored later.
+ */
+function escapeComments(content: string): {content: string, placeholders: Record<string, string>} {
+  const placeholders: Record<string, string> = {};
+  let commentCounter = 0;
+  let [openIndex, closeIndex] = findComment(content);
+
+  while (openIndex > -1 && closeIndex > -1) {
+    const placeholder = commentPlaceholderStart + (commentCounter++) + commentPlaceholderEnd;
+    placeholders[placeholder] = content.slice(openIndex, closeIndex);
+    content = content.slice(0, openIndex) + placeholder + content.slice(closeIndex);
+    [openIndex, closeIndex] = findComment(content);
+  }
+
+  return {content, placeholders};
+}
+
+/** Finds the start and end index of a comment in a file. */
+function findComment(content: string): [openIndex: number, closeIndex: number] {
+  // Add an extra new line at the end so that we can correctly capture single-line comments
+  // at the end of the file. It doesn't really matter that the end index will be out of bounds,
+  // because `String.prototype.slice` will clamp it to the string length.
+  content += '\n';
+
+  for (const [open, close] of commentPairs.entries()) {
+    const openIndex = content.indexOf(open);
+
+    if (openIndex > -1) {
+      const closeIndex = content.indexOf(close, openIndex + 1);
+      return closeIndex > -1 ? [openIndex, closeIndex + close.length] : [-1, -1];
+    }
+  }
+
+  return [-1, -1];
+}
+
+/** Restores the comments that have been escaped by `escapeComments`. */
+function restoreComments(content: string, placeholders: Record<string, string>): string {
+  Object.keys(placeholders).forEach(key => content = content.replace(key, placeholders[key]));
+  return content;
+}

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

@@ -281,23 +281,43 @@ describe('v12 theming API migration', () => {
     ]);
   });
 
-  it('should account for file headers placed aboved the @import statements', async () => {
-    writeLines(THEME_PATH, [
-      `/** This is a license. */`,
-      `@import './foo'`,
-      `@import '~@angular/material/theming';`,
-      `@include mat-core();`,
-    ]);
-
-    await runMigration();
-
-    expect(splitFile(THEME_PATH)).toEqual([
-      `/** This is a license. */`,
-      `@use '~@angular/material' as mat;`,
-      `@import './foo'`,
-      `@include mat.core();`,
-    ]);
-  });
+  it('should account for multi-line comment file headers placed aboved the @import statements',
+    async () => {
+      writeLines(THEME_PATH, [
+        `/** This is a license. */`,
+        `@import './foo'`,
+        `@import '~@angular/material/theming';`,
+        `@include mat-core();`,
+      ]);
+
+      await runMigration();
+
+      expect(splitFile(THEME_PATH)).toEqual([
+        `/** This is a license. */`,
+        `@use '~@angular/material' as mat;`,
+        `@import './foo'`,
+        `@include mat.core();`,
+      ]);
+    });
+
+  it('should account for single-line comment file headers placed aboved the @import statements',
+    async () => {
+      writeLines(THEME_PATH, [
+        `// This is a license.`,
+        `@import './foo'`,
+        `@import '~@angular/material/theming';`,
+        `@include mat-core();`,
+      ]);
+
+      await runMigration();
+
+      expect(splitFile(THEME_PATH)).toEqual([
+        `// This is a license.`,
+        `@use '~@angular/material' as mat;`,
+        `@import './foo'`,
+        `@include mat.core();`,
+      ]);
+    });
 
   it('should migrate multiple files within the same project', async () => {
     const componentPath = join(PROJECT_PATH, 'components/dialog.scss');
@@ -806,4 +826,52 @@ describe('v12 theming API migration', () => {
       `}`,
     ]);
   });
+
+  it('should not migrate commented out code', async () => {
+    writeLines(THEME_PATH, [
+      `// @import '~@angular/material/theming';`,
+      '/* @include mat-core(); */',
+    ]);
+
+    await runMigration();
+
+    expect(splitFile(THEME_PATH)).toEqual([
+      `// @import '~@angular/material/theming';`,
+      '/* @include mat-core(); */',
+    ]);
+  });
+
+  it('should not migrate single-line commented code at the end of the file', async () => {
+    writeLines(THEME_PATH, [
+      `// @import '~@angular/material/theming';`,
+      '// @include mat-core();',
+      '// @include mat-button-theme();',
+    ]);
+
+    await runMigration();
+
+    expect(splitFile(THEME_PATH)).toEqual([
+      `// @import '~@angular/material/theming';`,
+      '// @include mat-core();',
+      '// @include mat-button-theme();',
+    ]);
+  });
+
+  it('should handle mixed commented and non-commented content', async () => {
+    writeLines(THEME_PATH, [
+      `// @import '~@angular/material/theming';`,
+      '@include mat-core();',
+      '@include mat-button-theme();',
+    ]);
+
+    await runMigration();
+
+    expect(splitFile(THEME_PATH)).toEqual([
+      `// @import '~@angular/material/theming';`,
+      `@use '~@angular/material' as mat;`,
+      '@include mat.core();',
+      '@include mat.button-theme();',
+    ]);
+  });
+
 });