From 1e7d34a4bd7669443409d847f592f464450a6412 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Thu, 8 Apr 2021 21:26:09 +0200 Subject: [PATCH] fix(material/schematics): don't drop imports in files that do not use theming APIs I noticed this while trying out the theming migration against the docs site. The way the `theming-api` migration is set up means that we'll drop the Material imports as soon as we see them, but that doesn't guarantee that they're actually used further down in the file. These changes rework the logic so that we drop the imports after we've migrated the symbols. --- .../ng-generate/theming-api/index.spec.ts | 24 ++++++++- .../ng-generate/theming-api/migration.ts | 54 ++++++++++--------- 2 files changed, 53 insertions(+), 25 deletions(-) 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 a29891957985..89573d708095 100644 --- a/src/material/schematics/ng-generate/theming-api/index.spec.ts +++ b/src/material/schematics/ng-generate/theming-api/index.spec.ts @@ -101,6 +101,7 @@ describe('Material theming API schematic', () => { const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise(); expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([ `@use '~@angular/cdk' as cdk;`, + ``, `@include cdk.overlay();`, ``, `.my-dialog {`, @@ -142,8 +143,8 @@ describe('Material theming API schematic', () => { const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise(); expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([ - `@use '~@angular/cdk' as cdk;`, `@use '~@angular/material' as mat;`, + `@use '~@angular/cdk' as cdk;`, `@import './foo'`, ``, `@include cdk.overlay();`, @@ -484,4 +485,25 @@ describe('Material theming API schematic', () => { ]); }); + it('should not change files if they have an import, but do not use any symbols', async () => { + const app = await createTestApp(runner); + app.create('/theme.scss', [ + `@import '~@angular/material/theming';`, + ``, + `.my-dialog {`, + `color: red;`, + `}`, + ].join('\n')); + + 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;`, + `}`, + ]); + }); + + }); diff --git a/src/material/schematics/ng-generate/theming-api/migration.ts b/src/material/schematics/ng-generate/theming-api/migration.ts index 9e0acdc408ba..db9d3df473a9 100644 --- a/src/material/schematics/ng-generate/theming-api/migration.ts +++ b/src/material/schematics/ng-generate/theming-api/migration.ts @@ -114,33 +114,32 @@ export function migrateFileContent(content: string, oldCdkPrefix: string, newMaterialImportPath: string, newCdkImportPath: string): string { - // Drop the CDK imports and detect their namespaces. - const cdkResults = detectAndDropImports(content, oldCdkPrefix); - content = cdkResults.content; - - // Drop the Material imports and detect their namespaces. - const materialResults = detectAndDropImports(content, oldMaterialPrefix); - content = materialResults.content; - - // If nothing has changed, then the file doesn't import the Material theming API. - if (materialResults.hasChanged || cdkResults.hasChanged) { - // Replacing the imports may have resulted in leading whitespace. - content = content.replace(/^\s+/, ''); - content = migrateCdkSymbols(content, newCdkImportPath, cdkResults.namespaces); + 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+/, ''); + } } return content; } /** - * Finds all of the imports matching a prefix, removes them from - * the content string and returns some information about them. - * @param content Content from which to remove the imports. + * Counts the number of imports with a specific prefix and extracts their namespaces. + * @param content File content in which to look for imports. * @param prefix Prefix that the imports should start with. */ -function detectAndDropImports(content: string, prefix: string): - {content: string, hasChanged: boolean, namespaces: string[]} { +function detectImports(content: string, prefix: string): {imports: string[], namespaces: string[]} { 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.`); @@ -150,10 +149,13 @@ function detectAndDropImports(content: string, prefix: string): // Since we know that the library doesn't have any name collisions, we can treat all of these // namespaces as equivalent. const namespaces: string[] = []; + const imports: string[] = []; const pattern = new RegExp(`@(import|use) +['"]${escapeRegExp(prefix)}.*['"].*;?\n`, 'g'); - let hasChanged = false; + let match: RegExpExecArray | null = null; + + while (match = pattern.exec(content)) { + const [fullImport, type] = match; - content = content.replace(pattern, (fullImport, type: 'import' | 'use') => { if (type === 'use') { const namespace = extractNamespaceFromUseStatement(fullImport); @@ -162,11 +164,10 @@ function detectAndDropImports(content: string, prefix: string): } } - hasChanged = true; - return ''; - }); + imports.push(fullImport); + } - return {content, hasChanged, namespaces}; + return {imports, namespaces}; } /** Migrates the Material symbls in a file. */ @@ -303,6 +304,11 @@ function sortLengthDescending(a: string, b: string) { return b.length - a.length; } +/** Removes all strings from another string. */ +function removeStrings(content: string, toRemove: string[]): string { + return toRemove.reduce((accumulator, current) => accumulator.replace(current, ''), content); +} + /** Parses out the namespace from a Sass `@use` statement. */ function extractNamespaceFromUseStatement(fullImport: string): string { const closeQuoteIndex = Math.max(fullImport.lastIndexOf(`"`), fullImport.lastIndexOf(`'`));