Skip to content

Commit d030a87

Browse files
authored
fix(material/schematics): don't drop imports in files that do not use theming APIs (#22438)
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.
1 parent db44a8c commit d030a87

File tree

2 files changed

+53
-25
lines changed

2 files changed

+53
-25
lines changed

src/material/schematics/ng-generate/theming-api/index.spec.ts

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ describe('Material theming API schematic', () => {
101101
const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise();
102102
expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([
103103
`@use '~@angular/cdk' as cdk;`,
104+
``,
104105
`@include cdk.overlay();`,
105106
``,
106107
`.my-dialog {`,
@@ -142,8 +143,8 @@ describe('Material theming API schematic', () => {
142143

143144
const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise();
144145
expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([
145-
`@use '~@angular/cdk' as cdk;`,
146146
`@use '~@angular/material' as mat;`,
147+
`@use '~@angular/cdk' as cdk;`,
147148
`@import './foo'`,
148149
``,
149150
`@include cdk.overlay();`,
@@ -484,4 +485,25 @@ describe('Material theming API schematic', () => {
484485
]);
485486
});
486487

488+
it('should not change files if they have an import, but do not use any symbols', async () => {
489+
const app = await createTestApp(runner);
490+
app.create('/theme.scss', [
491+
`@import '~@angular/material/theming';`,
492+
``,
493+
`.my-dialog {`,
494+
`color: red;`,
495+
`}`,
496+
].join('\n'));
497+
498+
const tree = await runner.runSchematicAsync('theming-api', options, app).toPromise();
499+
expect(getFileContent(tree, '/theme.scss').split('\n')).toEqual([
500+
`@import '~@angular/material/theming';`,
501+
``,
502+
`.my-dialog {`,
503+
`color: red;`,
504+
`}`,
505+
]);
506+
});
507+
508+
487509
});

src/material/schematics/ng-generate/theming-api/migration.ts

Lines changed: 30 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -114,33 +114,32 @@ export function migrateFileContent(content: string,
114114
oldCdkPrefix: string,
115115
newMaterialImportPath: string,
116116
newCdkImportPath: string): string {
117-
// Drop the CDK imports and detect their namespaces.
118-
const cdkResults = detectAndDropImports(content, oldCdkPrefix);
119-
content = cdkResults.content;
120-
121-
// Drop the Material imports and detect their namespaces.
122-
const materialResults = detectAndDropImports(content, oldMaterialPrefix);
123-
content = materialResults.content;
124-
125-
// If nothing has changed, then the file doesn't import the Material theming API.
126-
if (materialResults.hasChanged || cdkResults.hasChanged) {
127-
// Replacing the imports may have resulted in leading whitespace.
128-
content = content.replace(/^\s+/, '');
129-
content = migrateCdkSymbols(content, newCdkImportPath, cdkResults.namespaces);
117+
const materialResults = detectImports(content, oldMaterialPrefix);
118+
const cdkResults = detectImports(content, oldCdkPrefix);
119+
120+
// If there are no imports, we don't need to go further.
121+
if (materialResults.imports.length > 0 || cdkResults.imports.length > 0) {
122+
const initialContent = content;
130123
content = migrateMaterialSymbols(content, newMaterialImportPath, materialResults.namespaces);
124+
content = migrateCdkSymbols(content, newCdkImportPath, cdkResults.namespaces);
125+
126+
// Only drop the imports if any of the symbols were used within the file.
127+
if (content !== initialContent) {
128+
content = removeStrings(content, materialResults.imports);
129+
content = removeStrings(content, cdkResults.imports);
130+
content = content.replace(/^\s+/, '');
131+
}
131132
}
132133

133134
return content;
134135
}
135136

136137
/**
137-
* Finds all of the imports matching a prefix, removes them from
138-
* the content string and returns some information about them.
139-
* @param content Content from which to remove the imports.
138+
* Counts the number of imports with a specific prefix and extracts their namespaces.
139+
* @param content File content in which to look for imports.
140140
* @param prefix Prefix that the imports should start with.
141141
*/
142-
function detectAndDropImports(content: string, prefix: string):
143-
{content: string, hasChanged: boolean, namespaces: string[]} {
142+
function detectImports(content: string, prefix: string): {imports: string[], namespaces: string[]} {
144143
if (prefix[prefix.length - 1] !== '/') {
145144
// Some of the logic further down makes assumptions about the import depth.
146145
throw Error(`Prefix "${prefix}" has to end in a slash.`);
@@ -150,10 +149,13 @@ function detectAndDropImports(content: string, prefix: string):
150149
// Since we know that the library doesn't have any name collisions, we can treat all of these
151150
// namespaces as equivalent.
152151
const namespaces: string[] = [];
152+
const imports: string[] = [];
153153
const pattern = new RegExp(`@(import|use) +['"]${escapeRegExp(prefix)}.*['"].*;?\n`, 'g');
154-
let hasChanged = false;
154+
let match: RegExpExecArray | null = null;
155+
156+
while (match = pattern.exec(content)) {
157+
const [fullImport, type] = match;
155158

156-
content = content.replace(pattern, (fullImport, type: 'import' | 'use') => {
157159
if (type === 'use') {
158160
const namespace = extractNamespaceFromUseStatement(fullImport);
159161

@@ -162,11 +164,10 @@ function detectAndDropImports(content: string, prefix: string):
162164
}
163165
}
164166

165-
hasChanged = true;
166-
return '';
167-
});
167+
imports.push(fullImport);
168+
}
168169

169-
return {content, hasChanged, namespaces};
170+
return {imports, namespaces};
170171
}
171172

172173
/** Migrates the Material symbls in a file. */
@@ -303,6 +304,11 @@ function sortLengthDescending(a: string, b: string) {
303304
return b.length - a.length;
304305
}
305306

307+
/** Removes all strings from another string. */
308+
function removeStrings(content: string, toRemove: string[]): string {
309+
return toRemove.reduce((accumulator, current) => accumulator.replace(current, ''), content);
310+
}
311+
306312
/** Parses out the namespace from a Sass `@use` statement. */
307313
function extractNamespaceFromUseStatement(fullImport: string): string {
308314
const closeQuoteIndex = Math.max(fullImport.lastIndexOf(`"`), fullImport.lastIndexOf(`'`));

0 commit comments

Comments
 (0)