From 8b6fdac1f7d1b67354293385337d475b98b813d7 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 3 May 2021 06:35:17 +0200 Subject: [PATCH] fix(material/schematics): migrate more cases in the theming API schematic (#22604) Includes the following improvements to the `themingApi` schematic: 1. No longer runs against files in the `node_modules`. This was happening by accident. 2. Remove the quotes around the values of `$mat-small` and `$mat-xsmall` since they were causing Sass syntax errors. 3. Migrates Material/CDK symbols, even if there are no imports for the old theming bundle. This allows us to handle symbols that were imported transitively. 4. Reverts the logic from #22438 that doesn't drop imports if there are no Material/CDK symbol usages within the file. I think that we want to do this after all, because the old import could slow down build times and we can be relatively certain that any usages have been migrated. Fixes #22599. (cherry picked from commit 94bb51880e2d5d89c5b84a7c05885403c727ee13) --- .../ng-generate/theming-api/config.ts | 4 +- .../ng-generate/theming-api/index.spec.ts | 41 +++++++++++++++++-- .../ng-generate/theming-api/index.ts | 2 +- .../ng-generate/theming-api/migration.ts | 32 ++++++++------- 4 files changed, 58 insertions(+), 21 deletions(-) diff --git a/src/material/schematics/ng-generate/theming-api/config.ts b/src/material/schematics/ng-generate/theming-api/config.ts index b13b2414632a..aa4fc415313a 100644 --- a/src/material/schematics/ng-generate/theming-api/config.ts +++ b/src/material/schematics/ng-generate/theming-api/config.ts @@ -103,8 +103,8 @@ export const cdkMixins: Record = { export const removedMaterialVariables: Record = { // Note: there's also a usage of a variable called `$pi`, but the name is short enough that // it matches things like `$mat-pink`. Don't migrate it since it's unlikely to be used. - 'mat-xsmall': `'max-width: 599px'`, - 'mat-small': `'max-width: 959px'`, + 'mat-xsmall': 'max-width: 599px', + 'mat-small': 'max-width: 959px', 'mat-toggle-padding': '8px', 'mat-toggle-size': '20px', 'mat-linear-out-slow-in-timing-function': 'cubic-bezier(0, 0, 0.2, 0.1)', diff --git a/src/material/schematics/ng-generate/theming-api/index.spec.ts b/src/material/schematics/ng-generate/theming-api/index.spec.ts index 56ce8288ef87..8514da7de586 100644 --- a/src/material/schematics/ng-generate/theming-api/index.spec.ts +++ b/src/material/schematics/ng-generate/theming-api/index.spec.ts @@ -195,6 +195,23 @@ describe('Material theming API schematic', () => { ]); }); + it('should migrate files that import the Material APIs transitively', async () => { + const app = await createTestApp(runner); + app.create('/theme.scss', [ + `@import 're-exports-material-symbols';`, + `@include mat-core();`, + `@include mat-button-theme();`, + ].join('\n')); + + const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise(); + expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([ + `@use '~@angular/material' as mat;`, + `@import 're-exports-material-symbols';`, + `@include mat.core();`, + `@include mat.button-theme();`, + ]); + }); + it('should allow an arbitrary number of spaces after @include and @import', async () => { const app = await createTestApp(runner); app.create('/theme.scss', [ @@ -485,7 +502,7 @@ describe('Material theming API schematic', () => { ]); }); - it('should not change files if they have an import, but do not use any symbols', async () => { + it('should drop the old import path even if the file is not using any symbols', async () => { const app = await createTestApp(runner); app.create('/theme.scss', [ `@import '~@angular/material/theming';`, @@ -497,8 +514,6 @@ describe('Material theming API schematic', () => { const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise(); expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([ - `@import '~@angular/material/theming';`, - ``, `.my-dialog {`, `color: red;`, `}`, @@ -537,7 +552,7 @@ describe('Material theming API schematic', () => { `transition: all 400ms cubic-bezier(0.25, 0.8, 0.25, 1);`, `}`, ``, - `@media ('max-width: 959px') {`, + `@media (max-width: 959px) {`, `.my-button-toggle {`, `height: 24px;`, `}`, @@ -571,4 +586,22 @@ describe('Material theming API schematic', () => { ]); }); + it('should not migrate files in the node_modules', async () => { + const app = await createTestApp(runner); + app.create('/node_modules/theme.scss', [ + `@import '~@angular/material/theming';`, + ``, + `@include mat-button-toggle-theme();`, + ``, + ].join('\n')); + + const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise(); + expect(getFileContent(tree, '/node_modules/theme.scss').split('\n')).toEqual([ + `@import '~@angular/material/theming';`, + ``, + `@include mat-button-toggle-theme();`, + ``, + ]); + }); + }); diff --git a/src/material/schematics/ng-generate/theming-api/index.ts b/src/material/schematics/ng-generate/theming-api/index.ts index b8c3a1143993..73cb0ba50018 100644 --- a/src/material/schematics/ng-generate/theming-api/index.ts +++ b/src/material/schematics/ng-generate/theming-api/index.ts @@ -14,7 +14,7 @@ import {migrateFileContent} from './migration'; export default function(_options: Schema): Rule { return (tree: Tree) => { tree.visit((path, entry) => { - if (extname(path) === '.scss') { + if (extname(path) === '.scss' && path.indexOf('node_modules') === -1) { const content = entry?.content.toString(); const migratedContent = content ? migrateFileContent(content, '~@angular/material/', '~@angular/cdk/', '~@angular/material', '~@angular/cdk') : content; diff --git a/src/material/schematics/ng-generate/theming-api/migration.ts b/src/material/schematics/ng-generate/theming-api/migration.ts index a46734677ca3..38a971d5ba7c 100644 --- a/src/material/schematics/ng-generate/theming-api/migration.ts +++ b/src/material/schematics/ng-generate/theming-api/migration.ts @@ -37,19 +37,21 @@ export function migrateFileContent(content: string, const materialResults = detectImports(content, oldMaterialPrefix); const cdkResults = detectImports(content, oldCdkPrefix); - // If there are no imports, we don't need to go further. - if (materialResults.imports.length > 0 || cdkResults.imports.length > 0) { - const initialContent = content; - content = migrateMaterialSymbols(content, newMaterialImportPath, materialResults.namespaces); - content = migrateCdkSymbols(content, newCdkImportPath, cdkResults.namespaces); - - // Only drop the imports if any of the symbols were used within the file. - if (content !== initialContent) { - content = removeStrings(content, materialResults.imports); - content = removeStrings(content, cdkResults.imports); - content = content.replace(/^\s+/, ''); - content = replaceRemovedVariables(content, removedMaterialVariables); - } + // 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 = replaceRemovedVariables(content, removedMaterialVariables); + + // We can assume that the migration has taken care of any Components symbols that were + // imported transitively so we can always drop the old imports. We also assume that imports + // to the new entry points have been added already. + if (materialResults.imports.length) { + content = removeStrings(content, materialResults.imports); + } + + if (cdkResults.imports.length) { + content = removeStrings(content, cdkResults.imports); } return content; @@ -227,7 +229,9 @@ function sortLengthDescending(a: string, b: string) { /** Removes all strings from another string. */ function removeStrings(content: string, toRemove: string[]): string { - return toRemove.reduce((accumulator, current) => accumulator.replace(current, ''), content); + return toRemove + .reduce((accumulator, current) => accumulator.replace(current, ''), content) + .replace(/^\s+/, ''); } /** Parses out the namespace from a Sass `@use` statement. */