Skip to content

Commit 326cca2

Browse files
committed
refactor(migrations): eagerly migrate Partial<T> references in google3 (angular#58049)
Instead of only migrating `Partial<T>` references to unwrap signal inputs when all members are migrated, we should do this, even if just a subset of inputs of the class are migrated. This is something we saw required manual fixups in google3— so this commit fixes this. PR Close angular#58049
1 parent d48aac8 commit 326cca2

File tree

7 files changed

+78
-66
lines changed

7 files changed

+78
-66
lines changed

packages/core/schematics/migrations/signal-migration/src/input_detection/directive_info.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,14 @@ export class DirectiveInfo {
3939
constructor(public clazz: ts.ClassDeclaration) {}
4040

4141
/**
42-
* Checks whether there are any incompatible inputs for the
42+
* Checks whether there are any migrated inputs for the
4343
* given class.
44+
*
45+
* Returns `false` if all inputs are incompatible.
4446
*/
45-
hasIncompatibleMembers(): boolean {
46-
return Array.from(this.inputFields.values()).some(({descriptor}) =>
47-
this.isInputMemberIncompatible(descriptor),
47+
hasMigratedFields(): boolean {
48+
return Array.from(this.inputFields.values()).some(
49+
({descriptor}) => !this.isInputMemberIncompatible(descriptor),
4850
);
4951
}
5052

packages/core/schematics/migrations/signal-migration/src/passes/9_migrate_ts_type_references.ts

Lines changed: 4 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,11 @@
77
*/
88

99
import {ImportManager} from '@angular/compiler-cli/src/ngtsc/translator';
10-
import assert from 'assert';
11-
import ts from 'typescript';
10+
import {ProgramInfo} from '../../../../utils/tsurge';
11+
import {migrateTypeScriptTypeReferences} from './reference_migration/migrate_ts_type_references';
1212
import {ReferenceMigrationHost} from './reference_migration/reference_migration_host';
1313
import {ClassFieldDescriptor} from './reference_resolution/known_fields';
14-
import {ProgramInfo, projectFile, Replacement, TextUpdate} from '../../../../utils/tsurge';
15-
import {isTsClassTypeReference, Reference} from './reference_resolution/reference_kinds';
14+
import {Reference} from './reference_resolution/reference_kinds';
1615

1716
/**
1817
* Migrates TypeScript "ts.Type" references. E.g.
@@ -26,58 +25,5 @@ export function pass9__migrateTypeScriptTypeReferences<D extends ClassFieldDescr
2625
importManager: ImportManager,
2726
info: ProgramInfo,
2827
) {
29-
const seenTypeNodes = new WeakSet<ts.TypeReferenceNode>();
30-
31-
for (const reference of references) {
32-
// This pass only deals with TS input class type references.
33-
if (!isTsClassTypeReference(reference)) {
34-
continue;
35-
}
36-
// Skip references to classes that are not fully migrated.
37-
if (!host.shouldMigrateReferencesToClass(reference.target)) {
38-
continue;
39-
}
40-
// Skip duplicate references. E.g. in batching.
41-
if (seenTypeNodes.has(reference.from.node)) {
42-
continue;
43-
}
44-
seenTypeNodes.add(reference.from.node);
45-
46-
if (reference.isPartialReference && reference.isPartOfCatalystFile) {
47-
assert(reference.from.node.typeArguments, 'Expected type arguments for partial reference.');
48-
assert(reference.from.node.typeArguments.length === 1, 'Expected an argument for reference.');
49-
50-
const firstArg = reference.from.node.typeArguments[0];
51-
const sf = firstArg.getSourceFile();
52-
// Naive detection of the import. Sufficient for this test file migration.
53-
const catalystImport = sf.text.includes(
54-
'google3/javascript/angular2/testing/catalyst/fake_async',
55-
)
56-
? 'google3/javascript/angular2/testing/catalyst/fake_async'
57-
: 'google3/javascript/angular2/testing/catalyst/async';
58-
59-
const unwrapImportExpr = importManager.addImport({
60-
exportModuleSpecifier: catalystImport,
61-
exportSymbolName: 'UnwrapSignalInputs',
62-
requestedFile: sf,
63-
});
64-
65-
host.replacements.push(
66-
new Replacement(
67-
projectFile(sf, info),
68-
new TextUpdate({
69-
position: firstArg.getStart(),
70-
end: firstArg.getStart(),
71-
toInsert: `${host.printer.printNode(ts.EmitHint.Unspecified, unwrapImportExpr, sf)}<`,
72-
}),
73-
),
74-
);
75-
host.replacements.push(
76-
new Replacement(
77-
projectFile(sf, info),
78-
new TextUpdate({position: firstArg.getEnd(), end: firstArg.getEnd(), toInsert: '>'}),
79-
),
80-
);
81-
}
82-
}
28+
migrateTypeScriptTypeReferences(host, references, importManager, info);
8329
}

packages/core/schematics/migrations/signal-migration/src/passes/reference_migration/migrate_ts_type_references.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
*/
88

99
import ts from 'typescript';
10-
import {KnownInputs} from '../../input_detection/known_inputs';
11-
import {MigrationResult} from '../../result';
1210
import {ProgramInfo, projectFile, Replacement, TextUpdate} from '../../../../../utils/tsurge';
1311
import assert from 'assert';
1412
import {ImportManager} from '@angular/compiler-cli/src/ngtsc/translator';
@@ -51,9 +49,15 @@ export function migrateTypeScriptTypeReferences<D extends ClassFieldDescriptor>(
5149

5250
const firstArg = reference.from.node.typeArguments[0];
5351
const sf = firstArg.getSourceFile();
52+
// Naive detection of the import. Sufficient for this test file migration.
53+
const catalystImport = sf.text.includes(
54+
'google3/javascript/angular2/testing/catalyst/fake_async',
55+
)
56+
? 'google3/javascript/angular2/testing/catalyst/fake_async'
57+
: 'google3/javascript/angular2/testing/catalyst/async';
5458

5559
const unwrapImportExpr = importManager.addImport({
56-
exportModuleSpecifier: 'google3/javascript/angular2/testing/catalyst',
60+
exportModuleSpecifier: catalystImport,
5761
exportSymbolName: 'UnwrapSignalInputs',
5862
requestedFile: sf,
5963
});

packages/core/schematics/migrations/signal-migration/src/phase_migrate.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ export function executeMigrationPhase(
4949
knownInputs.has(inputDescr) && knownInputs.get(inputDescr)!.isIncompatible() === false,
5050
shouldMigrateReferencesToClass: (clazz) =>
5151
knownInputs.getDirectiveInfoForClass(clazz) !== undefined &&
52-
knownInputs.getDirectiveInfoForClass(clazz)!.hasIncompatibleMembers() === false,
52+
knownInputs.getDirectiveInfoForClass(clazz)!.hasMigratedFields(),
5353
};
5454

5555
// Migrate passes.
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
// tslint:disable
2+
3+
import {Component, Input} from '@angular/core';
4+
5+
// @ts-ignore
6+
import {} from 'google3/javascript/angular2/testing/catalyst/fake_async';
7+
8+
function renderComponent(inputs: Partial<TestableSecondaryRangePicker> = {}) {}
9+
10+
@Component({
11+
standalone: false,
12+
jit: true,
13+
template: '<bla [(ngModel)]="incompatible">',
14+
})
15+
class TestableSecondaryRangePicker {
16+
@Input() bla = true;
17+
@Input() incompatible = true;
18+
}

packages/core/schematics/migrations/signal-migration/test/golden.txt

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,28 @@ it('should work', () => {
127127
} as Partial<UnwrapSignalInputs<MyComp>>;
128128
bootstrapTemplate('<my-comp [hello]="hello">', inputs);
129129
});
130+
@@@@@@ catalyst_test_partial.ts @@@@@@
131+
132+
// tslint:disable
133+
134+
import {Component, Input, input} from '@angular/core';
135+
136+
// @ts-ignore
137+
import {UnwrapSignalInputs} from 'google3/javascript/angular2/testing/catalyst/fake_async';
138+
139+
function renderComponent(inputs: Partial<UnwrapSignalInputs<TestableSecondaryRangePicker>> = {}) {}
140+
141+
@Component({
142+
standalone: false,
143+
jit: true,
144+
template: '<bla [(ngModel)]="incompatible">',
145+
})
146+
class TestableSecondaryRangePicker {
147+
readonly bla = input(true);
148+
// TODO: Skipped for migration because:
149+
// Your application code writes to the input. This prevents migration.
150+
@Input() incompatible = true;
151+
}
130152
@@@@@@ constructor_initializations.ts @@@@@@
131153

132154
// tslint:disable

packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,26 @@ it('should work', () => {
119119
} as Partial<UnwrapSignalInputs<MyComp>>;
120120
bootstrapTemplate('<my-comp [hello]="hello">', inputs);
121121
});
122+
@@@@@@ catalyst_test_partial.ts @@@@@@
123+
124+
// tslint:disable
125+
126+
import {Component, input} from '@angular/core';
127+
128+
// @ts-ignore
129+
import {UnwrapSignalInputs} from 'google3/javascript/angular2/testing/catalyst/fake_async';
130+
131+
function renderComponent(inputs: Partial<UnwrapSignalInputs<TestableSecondaryRangePicker>> = {}) {}
132+
133+
@Component({
134+
standalone: false,
135+
jit: true,
136+
template: '<bla [(ngModel)]="incompatible()">',
137+
})
138+
class TestableSecondaryRangePicker {
139+
readonly bla = input(true);
140+
readonly incompatible = input(true);
141+
}
122142
@@@@@@ constructor_initializations.ts @@@@@@
123143

124144
// tslint:disable

0 commit comments

Comments
 (0)