Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 49672c4

Browse filesBrowse files
surajy93pkozlowski-opensource
surajy93
authored and
pkozlowski-opensource
committed
fix(migrations): correctly migrate ngClass with mixed space-separated keys
Preserve NgClass import on partial migration and increment skippedNgClassCount when an unmigrable mixed binding is encountered.
1 parent 8201ceb commit 49672c4
Copy full SHA for 49672c4

5 files changed

+303-34Lines changed: 303 additions & 34 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎packages/core/schematics/BUILD.bazel‎

Copy file name to clipboardExpand all lines: packages/core/schematics/BUILD.bazel
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ rollup.rollup(
203203
"bundles",
204204
],
205205
visibility = [
206+
"//packages/core/schematics/ng-generate/ngclass-to-class-migration:__pkg__",
206207
"//packages/core/schematics/test:__pkg__",
207208
],
208209
)
Collapse file

‎packages/core/schematics/ng-generate/ngclass-to-class-migration/BUILD.bazel‎

Copy file name to clipboardExpand all lines: packages/core/schematics/ng-generate/ngclass-to-class-migration/BUILD.bazel
+30-2Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
load("//tools:defaults.bzl", "copy_to_bin", "ts_project")
1+
load("//tools:defaults.bzl", "copy_to_bin", "jasmine_test", "ts_project")
22

33
package(
44
default_visibility = [
@@ -14,7 +14,10 @@ copy_to_bin(
1414

1515
ts_project(
1616
name = "ngclass-to-class-migration",
17-
srcs = glob(["**/*.ts"]),
17+
srcs = glob(
18+
["**/*.ts"],
19+
exclude = ["*_spec.ts"],
20+
),
1821
data = ["schema.json"],
1922
deps = [
2023
"//:node_modules/@angular-devkit/schematics",
@@ -28,3 +31,28 @@ ts_project(
2831
"//packages/core/schematics/utils/tsurge/helpers/angular_devkit",
2932
],
3033
)
34+
35+
ts_project(
36+
name = "test_lib",
37+
testonly = True,
38+
srcs = glob(["*_spec.ts"]),
39+
deps = [
40+
"//:node_modules/@angular-devkit/core",
41+
"//:node_modules/@angular-devkit/schematics",
42+
"//packages/core/schematics/utils",
43+
"//packages/core/schematics/utils/tsurge",
44+
],
45+
)
46+
47+
jasmine_test(
48+
name = "test",
49+
timeout = "moderate",
50+
data = [
51+
":static_files",
52+
":test_lib",
53+
"//:node_modules/@angular/compiler",
54+
"//:node_modules/@angular/compiler-cli",
55+
"//packages/core/schematics:bundles",
56+
"//packages/core/schematics:schematics_jsons",
57+
],
58+
)
Collapse file

‎…/test/ngclass_to_class_migration_spec.ts‎ ‎…ation/ngclass_to_class_migration_spec.ts‎packages/core/schematics/test/ngclass_to_class_migration_spec.ts renamed to packages/core/schematics/ng-generate/ngclass-to-class-migration/ngclass_to_class_migration_spec.ts packages/core/schematics/test/ngclass_to_class_migration_spec.ts renamed to packages/core/schematics/ng-generate/ngclass-to-class-migration/ngclass_to_class_migration_spec.ts

Copy file name to clipboardExpand all lines: packages/core/schematics/ng-generate/ngclass-to-class-migration/ngclass_to_class_migration_spec.ts
+228-5Lines changed: 228 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ describe('NgClass migration', () => {
2525
return runner.runSchematic('ngclass-to-class', options, tree);
2626
}
2727

28-
const collectionJsonPath = resolve('../collection.json');
28+
const collectionJsonPath = resolve('../../collection.json');
2929
beforeEach(() => {
3030
runner = new SchematicTestRunner('test', collectionJsonPath);
3131
host = new TempScopedNodeJsSyncHost();
@@ -126,7 +126,6 @@ describe('NgClass migration', () => {
126126

127127
const content = tree.readContent('/app.component.ts');
128128

129-
// The object literal binding should be migrated.
130129
expect(content).toContain('[class.active]="isActive"');
131130
// The array binding should remain as [ngClass].
132131
expect(content).toContain("[ngClass]=\"['class-a', condition ? 'class-b' : '']\"");
@@ -505,7 +504,10 @@ describe('NgClass migration', () => {
505504
});
506505

507506
describe('Complex and multi-element migrations', () => {
508-
it('should migrate complex object literals with mixed class keys to [class] binding', async () => {
507+
it('should not migrate a space-separated key mixed with a regular key to [class] binding by default', async () => {
508+
// Regression test for https://github.com/angular/angular/issues/69833 — [class]="{...}"
509+
// object syntax does not support space-separated key names, so without
510+
// `migrateSpaceSeparatedKey` the binding must be left as [ngClass].
509511
writeFile(
510512
'/app.component.ts',
511513
`
@@ -525,8 +527,9 @@ describe('NgClass migration', () => {
525527
const content = tree.readContent('/app.component.ts');
526528

527529
expect(content).toContain(
528-
`<div [class]="{'class1 class2': condition, 'class3': anotherCondition}"></div>`,
530+
`<div [ngClass]="{'class1 class2': condition, 'class3': anotherCondition}"></div>`,
529531
);
532+
expect(content).not.toContain('[class]=');
530533
});
531534

532535
it('should migrate keys with extra whitespace for multiple conditions', async () => {
@@ -1039,7 +1042,7 @@ describe('migrateSpaceSeparatedKey option', () => {
10391042
return runner.runSchematic('ngclass-to-class', options, tree);
10401043
}
10411044

1042-
const collectionJsonPath = resolve('../collection.json');
1045+
const collectionJsonPath = resolve('../../collection.json');
10431046
beforeEach(() => {
10441047
runner = new SchematicTestRunner('test', collectionJsonPath);
10451048
host = new TempScopedNodeJsSyncHost();
@@ -1075,4 +1078,224 @@ describe('migrateSpaceSeparatedKey option', () => {
10751078

10761079
expect(content).toContain(`<div [class.class1]="condition" [class.class2]="condition"></div>`);
10771080
});
1081+
1082+
it('should expand mixed space-separated and regular keys individually', async () => {
1083+
// Regression test for https://github.com/angular/angular/issues/69833
1084+
// The object has one key with spaces ('class1 class2') and one plain key ('class3'),
1085+
// each with a different condition. The [class]="..." fallback cannot represent
1086+
// space-separated key names, so every entry must be emitted as [class.X]="condition".
1087+
writeFile(
1088+
'/app.component.ts',
1089+
`
1090+
import {Component} from '@angular/core';
1091+
import {NgClass} from '@angular/common';
1092+
@Component({
1093+
imports: [NgClass],
1094+
template: \`
1095+
<div [ngClass]="{'class1 class2': condition, 'class3': condition2}"></div>
1096+
\` })
1097+
export class Cmp {}
1098+
`,
1099+
);
1100+
1101+
await runMigration({migrateSpaceSeparatedKey: true});
1102+
1103+
const content = tree.readContent('/app.component.ts');
1104+
1105+
expect(content).toContain(
1106+
`<div [class.class1]="condition" [class.class2]="condition" [class.class3]="condition2"></div>`,
1107+
);
1108+
});
1109+
1110+
it('should not expand when a space-separated key is mixed with an empty-string key', async () => {
1111+
// Expanding every binding as [class.${key}] would produce the invalid `[class.]` binding
1112+
// for the empty key, so the whole binding must be left unchanged instead.
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': condition, '': condition2}"></div>
1122+
\` })
1123+
export class Cmp {}
1124+
`,
1125+
);
1126+
1127+
await runMigration({migrateSpaceSeparatedKey: true});
1128+
1129+
const content = tree.readContent('/app.component.ts');
1130+
1131+
expect(content).toContain(
1132+
`<div [ngClass]="{'class1 class2': condition, '': condition2}"></div>`,
1133+
);
1134+
expect(content).not.toContain('[class.]');
1135+
});
1136+
1137+
it('should not expand when a space-separated key expansion produces a duplicate class name', async () => {
1138+
// 'class1 class2': first expands to class1/class2, and the separate 'class1' key would
1139+
// collide with the expanded class1 binding, producing two conflicting [class.class1]
1140+
// bindings on the same element. The binding must be left unchanged instead.
1141+
writeFile(
1142+
'/app.component.ts',
1143+
`
1144+
import {Component} from '@angular/core';
1145+
import {NgClass} from '@angular/common';
1146+
@Component({
1147+
imports: [NgClass],
1148+
template: \`
1149+
<div [ngClass]="{'class1 class2': first, class1: second}"></div>
1150+
\` })
1151+
export class Cmp {}
1152+
`,
1153+
);
1154+
1155+
await runMigration({migrateSpaceSeparatedKey: true});
1156+
1157+
const content = tree.readContent('/app.component.ts');
1158+
1159+
expect(content).toContain(`<div [ngClass]="{'class1 class2': first, class1: second}"></div>`);
1160+
});
1161+
1162+
it('should not expand a space-separated key whose expanded class name contains a dot', async () => {
1163+
// `[class.a.b]` is parsed by Angular as binding to property `a`, silently discarding
1164+
// everything after the first dot — expanding 'a.b c' would produce a binding that
1165+
// silently applies the wrong class. The whole binding must be left unchanged instead.
1166+
writeFile(
1167+
'/app.component.ts',
1168+
`
1169+
import {Component} from '@angular/core';
1170+
import {NgClass} from '@angular/common';
1171+
@Component({
1172+
imports: [NgClass],
1173+
template: \`
1174+
<div [ngClass]="{'a.b c': condition, 'd': condition2}"></div>
1175+
\` })
1176+
export class Cmp {}
1177+
`,
1178+
);
1179+
1180+
await runMigration({migrateSpaceSeparatedKey: true});
1181+
1182+
const content = tree.readContent('/app.component.ts');
1183+
1184+
expect(content).toContain(`<div [ngClass]="{'a.b c': condition, 'd': condition2}"></div>`);
1185+
expect(content).not.toContain('[class.a');
1186+
});
1187+
1188+
it('should treat a tab as a class-name separator', async () => {
1189+
writeFile(
1190+
'/app.component.ts',
1191+
`
1192+
import {Component} from '@angular/core';
1193+
import {NgClass} from '@angular/common';
1194+
@Component({
1195+
imports: [NgClass],
1196+
template: \`
1197+
<div [ngClass]="{'class1\\tclass2': condition}"></div>
1198+
\` })
1199+
export class Cmp {}
1200+
`,
1201+
);
1202+
1203+
await runMigration({migrateSpaceSeparatedKey: true});
1204+
1205+
const content = tree.readContent('/app.component.ts');
1206+
1207+
expect(content).toContain(`<div [class.class1]="condition" [class.class2]="condition"></div>`);
1208+
});
1209+
1210+
it('should treat a newline as a class-name separator', async () => {
1211+
writeFile(
1212+
'/app.component.ts',
1213+
`
1214+
import {Component} from '@angular/core';
1215+
import {NgClass} from '@angular/common';
1216+
@Component({
1217+
imports: [NgClass],
1218+
template: \`
1219+
<div [ngClass]="{'class1\\nclass2': condition}"></div>
1220+
\` })
1221+
export class Cmp {}
1222+
`,
1223+
);
1224+
1225+
await runMigration({migrateSpaceSeparatedKey: true});
1226+
1227+
const content = tree.readContent('/app.component.ts');
1228+
1229+
expect(content).toContain(`<div [class.class1]="condition" [class.class2]="condition"></div>`);
1230+
});
1231+
1232+
it('should not expand when a shorthand property collides with an expanded space-separated key', async () => {
1233+
// The shorthand `class1` binding and the `class1` produced by splitting
1234+
// 'class1 class2' would collide, producing two conflicting [class.class1] bindings.
1235+
writeFile(
1236+
'/app.component.ts',
1237+
`
1238+
import {Component} from '@angular/core';
1239+
import {NgClass} from '@angular/common';
1240+
@Component({
1241+
imports: [NgClass],
1242+
template: \`
1243+
<div [ngClass]="{class1, 'class1 class2': condition}"></div>
1244+
\` })
1245+
export class Cmp {}
1246+
`,
1247+
);
1248+
1249+
await runMigration({migrateSpaceSeparatedKey: true});
1250+
1251+
const content = tree.readContent('/app.component.ts');
1252+
1253+
expect(content).toContain(`<div [ngClass]="{class1, 'class1 class2': condition}"></div>`);
1254+
});
1255+
1256+
it('should expand a space-separated key mixed with a numeric-looking class name', async () => {
1257+
writeFile(
1258+
'/app.component.ts',
1259+
`
1260+
import {Component} from '@angular/core';
1261+
import {NgClass} from '@angular/common';
1262+
@Component({
1263+
imports: [NgClass],
1264+
template: \`
1265+
<div [ngClass]="{'1foo bar': condition}"></div>
1266+
\` })
1267+
export class Cmp {}
1268+
`,
1269+
);
1270+
1271+
await runMigration({migrateSpaceSeparatedKey: true});
1272+
1273+
const content = tree.readContent('/app.component.ts');
1274+
1275+
expect(content).toContain(`<div [class.1foo]="condition" [class.bar]="condition"></div>`);
1276+
});
1277+
1278+
it('should not migrate when a computed property key is mixed with a space-separated key', async () => {
1279+
writeFile(
1280+
'/app.component.ts',
1281+
`
1282+
import {Component} from '@angular/core';
1283+
import {NgClass} from '@angular/common';
1284+
@Component({
1285+
imports: [NgClass],
1286+
template: \`
1287+
<div [ngClass]="{[dynamicKey]: condition, 'class1 class2': condition2}"></div>
1288+
\` })
1289+
export class Cmp {}
1290+
`,
1291+
);
1292+
1293+
await runMigration({migrateSpaceSeparatedKey: true});
1294+
1295+
const content = tree.readContent('/app.component.ts');
1296+
1297+
expect(content).toContain(
1298+
`<div [ngClass]="{[dynamicKey]: condition, 'class1 class2': condition2}"></div>`,
1299+
);
1300+
});
10781301
});

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.