Skip to content

Commit 19f989e

Browse files
aminesbdevaminesbdev
committed
fix(migrations): match the quote character to the migrated expression
The ngclass-to-class migration always emitted its replacement as `name="<value>"`. When the original attribute was single-quoted, the value can contain a double quote, which terminates the attribute early and produces invalid HTML: <div [ngClass]='{"a": isA, "b": isB}'> <div [class]="{"a": isA, "b": isB}"> <- before <div [class]='{"a": isA, "b": isB}'> <- after <div [ngClass]='{active: mode === "on"}'> <div [class.active]="mode === "on""> <- before <div [class.active]='mode === "on"'> <- after The quote character is now picked to match the value. A value can only contain a double quote if it came from a single-quoted attribute, in which case it contains no single quote, so single quotes are always safe and no case is left unhandled. That also removes the need for the bail-out on double quotes in the static string and array path, so `[ngClass]='["a", "b"]'` now migrates as well. Drops the sparse array handling along the way. `parseLiteralArray` calls `parsePipe()` as soon as the token following a comma is not `]`, so `[ngClass]="['a', , 'b']"` fails to parse with `Unexpected token ,` and never reaches the migration.
1 parent 5e328f7 commit 19f989e

2 files changed

Lines changed: 123 additions & 19 deletions

File tree

‎packages/core/schematics/ng-generate/ngclass-to-class-migration/ngclass_to_class_migration_spec.ts‎

Lines changed: 108 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1046,7 +1046,7 @@ describe('NgClass migration', () => {
10461046
expect(content).toContain(`<div [ngClass]="['class1 class2', 'class3']"></div>`);
10471047
});
10481048

1049-
it('should not migrate a double-quoted literal that would break the attribute', async () => {
1049+
it('should migrate a double-quoted literal into a single-quoted attribute', async () => {
10501050
writeFile(
10511051
'/app.component.ts',
10521052
`
@@ -1064,7 +1064,113 @@ describe('NgClass migration', () => {
10641064
await runMigration();
10651065

10661066
const content = tree.readContent('/app.component.ts');
1067-
expect(content).toContain(`<div [ngClass]='"class1 class2"'></div>`);
1067+
expect(content).toContain(`<div [class]='"class1 class2"'></div>`);
1068+
});
1069+
1070+
it('should migrate an array of double-quoted entries into a single-quoted attribute', async () => {
1071+
writeFile(
1072+
'/app.component.ts',
1073+
`
1074+
import {Component} from '@angular/core';
1075+
import {NgClass} from '@angular/common';
1076+
@Component({
1077+
imports: [NgClass],
1078+
template: \`
1079+
<div [ngClass]='["class1", "class2"]'></div>
1080+
\` })
1081+
export class Cmp {}
1082+
`,
1083+
);
1084+
1085+
await runMigration();
1086+
1087+
const content = tree.readContent('/app.component.ts');
1088+
expect(content).toContain(`<div [class]='["class1", "class2"]'></div>`);
1089+
});
1090+
1091+
it('should migrate a template literal to [class]', async () => {
1092+
writeFile(
1093+
'/app.component.ts',
1094+
`
1095+
import {Component} from '@angular/core';
1096+
import {NgClass} from '@angular/common';
1097+
@Component({
1098+
imports: [NgClass],
1099+
template: \`
1100+
<div [ngClass]="\`class1 class2\`"></div>
1101+
\` })
1102+
export class Cmp {}
1103+
`,
1104+
);
1105+
1106+
await runMigration();
1107+
1108+
const content = tree.readContent('/app.component.ts');
1109+
expect(content).toContain('<div [class]="`class1 class2`"></div>');
1110+
});
1111+
1112+
it('should remove the NgClass import once a string literal binding is migrated', async () => {
1113+
writeFile(
1114+
'/app.component.ts',
1115+
`
1116+
import {Component} from '@angular/core';
1117+
import {NgClass} from '@angular/common';
1118+
@Component({
1119+
imports: [NgClass],
1120+
template: \`
1121+
<div [ngClass]="'class1 class2'"></div>
1122+
\` })
1123+
export class Cmp {}
1124+
`,
1125+
);
1126+
1127+
await runMigration();
1128+
1129+
const content = tree.readContent('/app.component.ts');
1130+
expect(content).toContain(`<div [class]="'class1 class2'"></div>`);
1131+
expect(content).not.toContain("import {NgClass} from '@angular/common';");
1132+
});
1133+
1134+
it('should migrate an object literal with double-quoted keys into a single-quoted attribute', async () => {
1135+
writeFile(
1136+
'/app.component.ts',
1137+
`
1138+
import {Component} from '@angular/core';
1139+
import {NgClass} from '@angular/common';
1140+
@Component({
1141+
imports: [NgClass],
1142+
template: \`
1143+
<div [ngClass]='{"class1": isA, "class2": isB}'></div>
1144+
\` })
1145+
export class Cmp {}
1146+
`,
1147+
);
1148+
1149+
await runMigration();
1150+
1151+
const content = tree.readContent('/app.component.ts');
1152+
expect(content).toContain(`<div [class]='{"class1": isA, "class2": isB}'></div>`);
1153+
});
1154+
1155+
it('should migrate a condition containing a double-quoted string into a single-quoted attribute', async () => {
1156+
writeFile(
1157+
'/app.component.ts',
1158+
`
1159+
import {Component} from '@angular/core';
1160+
import {NgClass} from '@angular/common';
1161+
@Component({
1162+
imports: [NgClass],
1163+
template: \`
1164+
<div [ngClass]='{active: mode === "on"}'></div>
1165+
\` })
1166+
export class Cmp {}
1167+
`,
1168+
);
1169+
1170+
await runMigration();
1171+
1172+
const content = tree.readContent('/app.component.ts');
1173+
expect(content).toContain(`<div [class.active]='mode === "on"'></div>`);
10681174
});
10691175

10701176
it('should not migrate dynamic variable bindings', async () => {

‎packages/core/schematics/ng-generate/ngclass-to-class-migration/util.ts‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -304,7 +304,7 @@ export class NgClassCollector extends RecursiveVisitor {
304304
this.replacements.push({
305305
start: attr.sourceSpan.start.offset,
306306
end: attr.sourceSpan.end.offset,
307-
replacement: `[class]="${staticLiteral}"`,
307+
replacement: buildAttribute('[class]', staticLiteral),
308308
});
309309
continue;
310310
}
@@ -329,7 +329,7 @@ export class NgClassCollector extends RecursiveVisitor {
329329
replacement = '[class]=""';
330330
} else {
331331
// Normal single condition: use [class.className]="condition"
332-
replacement = `[class.${key}]="${value}"`;
332+
replacement = buildAttribute(`[class.${key}]`, value);
333333
}
334334
} else {
335335
// Multiple bindings. If any original key contained spaces, [class]="{...}" object
@@ -349,7 +349,7 @@ export class NgClassCollector extends RecursiveVisitor {
349349

350350
if (config.migrateSpaceSeparatedKey && canExpand) {
351351
replacement = staticMatch
352-
.map(({key, value}) => `[class.${key}]="${value}"`)
352+
.map(({key, value}) => buildAttribute(`[class.${key}]`, value))
353353
.join(' ');
354354
} else {
355355
// Cannot produce valid [class]="..." output — leave binding as-is.
@@ -358,7 +358,7 @@ export class NgClassCollector extends RecursiveVisitor {
358358
}
359359
} else {
360360
// All keys are single class names: [class]="{'cls1': cond1, 'cls2': cond2}" is valid.
361-
replacement = `[class]="${expr}"`;
361+
replacement = buildAttribute('[class]', expr);
362362
}
363363
}
364364

@@ -396,13 +396,6 @@ export class NgClassCollector extends RecursiveVisitor {
396396
function tryParseStaticStringOrArrayLiteral(expr: string): string | null {
397397
const trimmedExpr = expr.trim();
398398

399-
// The replacement is emitted as `[class]="<expr>"`, so an expression containing a double
400-
// quote (e.g. the double-quoted string in `[ngClass]='["a", "b"]'`) would terminate the
401-
// attribute early and produce invalid HTML. Leave those bindings alone.
402-
if (trimmedExpr.includes('"')) {
403-
return null;
404-
}
405-
406399
let initializer: ts.Expression;
407400
try {
408401
const sourceFile = ts.createSourceFile(
@@ -443,12 +436,6 @@ function tryParseStaticStringOrArrayLiteral(expr: string): string | null {
443436
}
444437

445438
for (const element of initializer.elements) {
446-
// A hole in a sparse array (`['a', , 'b']`) parses as an omitted expression. `NgClass`
447-
// skips the resulting `undefined`, and so does `[class]`, so it is safe to carry over.
448-
if (ts.isOmittedExpression(element)) {
449-
continue;
450-
}
451-
452439
if (!isStaticStringLiteral(element)) {
453440
return null;
454441
}
@@ -463,6 +450,17 @@ function tryParseStaticStringOrArrayLiteral(expr: string): string | null {
463450
return trimmedExpr;
464451
}
465452

453+
/**
454+
* Builds an attribute for the migrated template. The value is emitted verbatim, so the quote
455+
* character has to match it: a value containing a double quote (which can only come from a
456+
* single-quoted attribute such as `[ngClass]='["a", "b"]'`, and therefore never contains a
457+
* single quote) has to be wrapped in single quotes, or it would terminate the attribute early
458+
* and produce invalid HTML.
459+
*/
460+
function buildAttribute(name: string, value: string): string {
461+
return value.includes('"') ? `${name}='${value}'` : `${name}="${value}"`;
462+
}
463+
466464
/** Whether the expression is a string literal with no interpolation. */
467465
function isStaticStringLiteral(
468466
node: ts.Expression,

0 commit comments

Comments
 (0)