Skip to content

Commit a34a284

Browse files
committed
fix(material/schematics): avoid re-entrant processing in MDC style migration
We're mutating the AST in a PostCSS processor during the MDC migration when fixing user's code. The problem is that when PostCSS detects that the AST was changed, it will re-run the processors until there are no more changes left to make. This is problematic for migrations like typography hierarchy, because some legacy classes are swapped with non-legacy ones which causes PostCSS to re-run and apply a new incorrect migration. These changes aim to avoid the issue preventing such re-entrant style migrations. (cherry picked from commit e80929d)
1 parent 32827ac commit a34a284

File tree

3 files changed

+76
-18
lines changed

3 files changed

+76
-18
lines changed

src/material/schematics/ng-generate/mdc-migration/rules/components/typography-hierarchy/typography-hierarchy-styles.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,15 @@ export class TypographyHierarchyStylesMigrator extends StyleMigrator {
2525
super();
2626

2727
RENAMED_TYPOGRAPHY_CLASSES.forEach((newClass, oldClass) => {
28-
this.classChanges.push({new: '.' + newClass, old: '.' + oldClass});
28+
// Some classes get renamed to each other. E.g. `subheading-1` -> `body-1` -> `body-2`.
29+
// PostCSS will re-run its processors whenever an AST is mutated which means that we'll
30+
// either end up with an incorrect result or potentially fall into an infinite loop. Wrap
31+
// the risky classes in a special string that will be stripped out later to avoid the issue.
32+
const wrappedNewClass = RENAMED_TYPOGRAPHY_CLASSES.has(newClass)
33+
? `.${StyleMigrator.wrapValue(newClass)}`
34+
: `.${newClass}`;
35+
36+
this.classChanges.push({new: wrappedNewClass, old: '.' + oldClass});
2937
});
3038
}
3139
}

src/material/schematics/ng-generate/mdc-migration/rules/style-migrator.ts

Lines changed: 46 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,26 @@ export abstract class StyleMigrator {
4646
/** The prefix of classes that are specific to the old components */
4747
abstract deprecatedPrefixes: string[];
4848

49+
/**
50+
* Data structure used to track which migrators have been applied to an AST node
51+
* already so they don't have to be re-run when PostCSS detects changes in the AST.
52+
*/
53+
private _processedNodes = new WeakMap<postcss.Node, Set<string>>();
54+
55+
/**
56+
* Wraps a value in a placeholder string to prevent it
57+
* from being matched multiple times in a migration.
58+
*/
59+
static wrapValue(value: string): string {
60+
const escapeString = '__NG_MDC_MIGRATION_PLACEHOLDER__';
61+
return `${escapeString}${value}${escapeString}`;
62+
}
63+
64+
/** Unwraps all the values that we wrapped by `wrapValue`. */
65+
static unwrapAllValues(content: string): string {
66+
return content.replace(/__NG_MDC_MIGRATION_PLACEHOLDER__/g, '');
67+
}
68+
4969
/**
5070
* Returns whether the given at-include at-rule is a use of a legacy mixin for this component.
5171
*
@@ -66,6 +86,12 @@ export abstract class StyleMigrator {
6686
* @returns the mixin change object or null if not found
6787
*/
6888
getMixinChange(namespace: string, atRule: postcss.AtRule): MixinChange | null {
89+
const processedKey = `mixinChange-${namespace}`;
90+
91+
if (this._nodeIsProcessed(atRule, processedKey)) {
92+
return null;
93+
}
94+
6995
const change = this.mixinChanges.find(c => {
7096
return atRule.params.includes(`${namespace}.${c.old}`);
7197
});
@@ -94,6 +120,7 @@ export abstract class StyleMigrator {
94120
});
95121
}
96122

123+
this._trackProcessedNode(atRule, processedKey);
97124
return {old: change.old, new: replacements.length ? replacements : null};
98125
}
99126

@@ -117,11 +144,14 @@ export abstract class StyleMigrator {
117144
* @param rule a postcss rule.
118145
*/
119146
replaceLegacySelector(rule: postcss.Rule): void {
120-
for (let i = 0; i < this.classChanges.length; i++) {
121-
const change = this.classChanges[i];
122-
if (rule.selector?.match(change.old + END_OF_SELECTOR_REGEX)) {
123-
rule.selector = rule.selector.replace(change.old, change.new);
147+
if (!this._nodeIsProcessed(rule, 'replaceLegacySelector')) {
148+
for (let i = 0; i < this.classChanges.length; i++) {
149+
const change = this.classChanges[i];
150+
if (rule.selector?.match(change.old + END_OF_SELECTOR_REGEX)) {
151+
rule.selector = rule.selector.replace(change.old, change.new);
152+
}
124153
}
154+
this._trackProcessedNode(rule, 'replaceLegacySelector');
125155
}
126156
}
127157

@@ -137,4 +167,16 @@ export abstract class StyleMigrator {
137167
rule.selector.includes(deprecatedPrefix),
138168
);
139169
}
170+
171+
/** Tracks that a node has been processed by a specific action. */
172+
private _trackProcessedNode(node: postcss.Node, action: string) {
173+
const appliedActions = this._processedNodes.get(node) || new Set();
174+
appliedActions.add(action);
175+
this._processedNodes.set(node, appliedActions);
176+
}
177+
178+
/** Checks whether a node has been processed by an action in this migrator. */
179+
private _nodeIsProcessed(node: postcss.Node, action: string) {
180+
return !!this._processedNodes.get(node)?.has(action);
181+
}
140182
}

src/material/schematics/ng-generate/mdc-migration/rules/theming-styles.ts

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import * as postcss from 'postcss';
1212
import * as scss from 'postcss-scss';
1313
import {ComponentMigrator, MIGRATORS, PERMANENT_MIGRATORS} from '.';
1414
import {RENAMED_TYPOGRAPHY_LEVELS} from './components/typography-hierarchy/constants';
15+
import {StyleMigrator} from './style-migrator';
1516

1617
const COMPONENTS_MIXIN_NAME = /\.([^(;]*)/;
1718

@@ -38,29 +39,33 @@ export class ThemingStylesMigration extends Migration<ComponentMigrator[], Schem
3839
{
3940
postcssPlugin: 'theming-styles-migration-plugin',
4041
AtRule: {
41-
use: this.atUseHandler.bind(this),
42-
include: this.atIncludeHandler.bind(this),
42+
use: this._atUseHandler.bind(this),
43+
include: atRule => this._atIncludeHandler(atRule),
4344
},
44-
Rule: this.ruleHandler.bind(this),
45+
Rule: rule => this._ruleHandler(rule),
4546
},
4647
]);
4748

4849
try {
49-
return processor.process(styles, {syntax: scss}).toString();
50+
const result = processor.process(styles, {syntax: scss}).toString();
51+
// PostCSS will re-run the processors if it detects that an AST has been mutated. We want to
52+
// avoid this when removing the unwrapped values, because it may cause them to be re-migrated
53+
// incorrectly. This is achieved by unwrapping after the AST has been converted to a string.
54+
return result === styles ? styles : StyleMigrator.unwrapAllValues(result);
5055
} catch (e) {
5156
this.context.logger.error(`${e}`);
5257
this.context.logger.warn(`Failed to process stylesheet: ${filename} (see error above).`);
5358
return styles;
5459
}
5560
}
5661

57-
atUseHandler(atRule: postcss.AtRule) {
62+
private _atUseHandler(atRule: postcss.AtRule) {
5863
if (isAngularMaterialImport(atRule)) {
5964
this._namespace = parseNamespace(atRule);
6065
}
6166
}
6267

63-
atIncludeHandler(atRule: postcss.AtRule) {
68+
private _atIncludeHandler(atRule: postcss.AtRule) {
6469
const migrator = this.upgradeData.find(m => {
6570
return m.styles.isLegacyMixin(this._namespace, atRule);
6671
});
@@ -98,24 +103,27 @@ export class ThemingStylesMigration extends Migration<ComponentMigrator[], Schem
98103
return this.upgradeData.length !== MIGRATORS.length + PERMANENT_MIGRATORS.length;
99104
}
100105

101-
ruleHandler(rule: postcss.Rule) {
102-
let isLegacySelector;
103-
let isDeprecatedSelector;
106+
private _ruleHandler(rule: postcss.Rule) {
107+
let isLegacySelector = false;
108+
let isDeprecatedSelector = false;
104109

105110
const migrator = this.upgradeData.find(m => {
106111
isLegacySelector = m.styles.isLegacySelector(rule);
107112
isDeprecatedSelector = m.styles.isDeprecatedSelector(rule);
108113
return isLegacySelector || isDeprecatedSelector;
109114
});
110115

116+
if (!migrator) {
117+
return;
118+
}
119+
111120
if (isLegacySelector) {
112-
migrator?.styles.replaceLegacySelector(rule);
121+
migrator.styles.replaceLegacySelector(rule);
113122
} else if (isDeprecatedSelector) {
114123
addCommentBeforeNode(
115124
rule,
116-
'TODO(mdc-migration): The following rule targets internal classes of ' +
117-
migrator?.component +
118-
' that may no longer apply for the MDC version.',
125+
`TODO(mdc-migration): The following rule targets internal classes of ${migrator.component} ` +
126+
`that may no longer apply for the MDC version.`,
119127
);
120128
}
121129
}

0 commit comments

Comments
 (0)