8000 fix(migrations): avoid migrating the same class multiple times in sta… · angular/angular@44d095a · GitHub
[go: up one dir, main page]
Skip to content

Commit 44d095a

Browse files
crisbetoAndrewKushnir
crisbeto
authored and
AndrewKushnir
committed
fix(migrations): avoid migrating the same class multiple times in standalone migration (#49245)
If a class is declared in multiple modules, the standalone migration may end up generating invalid code. While declaring a class in multiple modules is an error, it can happen with modules in tests. These changes avoid the issue by using a `Set` to track the classes being migrated. PR Close #49245
1 parent 600fd12 commit 44d095a

File tree

2 files changed

+54
-60
lines changed

2 files changed

+54
-60
lines changed

‎packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ interface BootstrapCallAnalysis {
2929
/** Component that the module is bootstrapping. */
3030
component: NamedClassDeclaration;
3131
/** Classes declared by the bootstrapped module. */
32-
declarations: Reference<ts.ClassDeclaration>[];
32+
declarations: ts.ClassDeclaration[];
3333
}
3434

3535
export function toStandaloneBootstrap(
@@ -42,8 +42,8 @@ export function toStandaloneBootstrap(
4242
const referenceResolver =
4343
new ReferenceResolver(program, host, rootFileNames, basePath, referenceLookupExcludedFiles);
4444
const bootstrapCalls: BootstrapCallAnalysis[] = [];
45-
const testObjects: ts.ObjectLiteralExpression[] = [];
46-
const allDeclarations: Reference<ts.ClassDeclaration>[] = [];
45+
const testObjects = new Set<ts.ObjectLiteralExpression>();
46+
const allDeclarations = new Set<ts.ClassDeclaration>();
4747

4848
for (const sourceFile of sourceFiles) {
4949
sourceFile.forEachChild(function walk(node) {
@@ -59,11 +59,11 @@ export function toStandaloneBootstrap(
5959
node.forEachChild(walk);
6060
});
6161

62-
testObjects.push(...findTestObjectsToMigrate(sourceFile, typeChecker));
62+
findTestObjectsToMigrate(sourceFile, typeChecker).forEach(obj => testObjects.add(obj));
6363
}
6464

6565
for (const call of bootstrapCalls) {
66-
allDeclarations.push(...call.declarations);
66+
call.declarations.forEach(decl => allDeclarations.add(decl));
6767
migrateBootstrapCall(call, tracker, referenceResolver, typeChecker, printer);
6868
}
6969

‎packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts

Lines changed: 49 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty,
2222
* are going to be added to the imports of a component.
2323
*/
2424
export type ComponentImportsRemapper =
25-
(imports: PotentialImport[], component: Reference<ts.ClassDeclaration>) => PotentialImport[];
25+
(imports: PotentialImport[], component: ts.ClassDeclaration) => PotentialImport[];
2626

2727
/**
2828
* Converts all declarations in the specified files to standalone.
@@ -39,9 +39,9 @@ export function toStandalone(
3939
componentImportRemapper?: ComponentImportsRemapper): ChangesByFile {
4040
const templateTypeChecker = program.compiler.getTemplateTypeChecker();
4141
const typeChecker = program.getTsProgram().getTypeChecker();
42-
const modulesToMigrate: ts.ClassDeclaration[] = [];
43-
const testObjectsToMigrate: ts.ObjectLiteralExpression[] = [];
44-
const declarations: Reference<ts.ClassDeclaration>[] = [];
42+
const modulesToMigrate = new Set<ts.ClassDeclaration>();
43+
const testObjectsToMigrate = new Set<ts.ObjectLiteralExpression>();
44+
const declarations = new Set<ts.ClassDeclaration>();
4545
const tracker = new ChangeTracker(printer, fileImportRemapper);
4646

4747
for (const sourceFile of sourceFiles) {
@@ -54,12 +54,12 @@ export function toStandalone(
5454
allModuleDeclarations, module, templateTypeChecker, typeChecker);
5555

5656
if (unbootstrappedDeclarations.length > 0) {
57-
modulesToMigrate.push(module);
58-
declarations.push(...unbootstrappedDeclarations);
57+
modulesToMigrate.add(module);
58+
unbootstrappedDeclarations.forEach(decl => declarations.add(decl));
5959
}
6060
}
6161

62-
testObjectsToMigrate.push(...testObjects);
62+
testObjects.forEach(obj => testObjectsToMigrate.add(obj));
6363
}
6464

6565
for (const declaration of declarations) {
@@ -78,24 +78,23 @@ export function toStandalone(
7878

7979
/**
8080
* Converts a single declaration defined through an NgModule to standalone.
81-
* @param ref References to the declaration being converted.
81+
* @param decl Declaration being converted.
8282
* @param tracker Tracker used to track the file changes.
8383
* @param allDeclarations All the declarations that are being converted as a part of this migration.
8484
* @param typeChecker
8585
* @param importRemapper
8686
*/
8787
export function convertNgModuleDeclarationToStandalone(
88-
ref: Reference<ts.ClassDeclaration>, allDeclarations: Reference<ts.ClassDeclaration>[],
89-
tracker: ChangeTracker, typeChecker: TemplateTypeChecker,
90-
importRemapper?: ComponentImportsRemapper): void {
91-
const directiveMeta = typeChecker.getDirectiveMetadata(ref.node);
88+
decl: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
89+
typeChecker: TemplateTypeChecker, importRemapper?: ComponentImportsRemapper): void {
90+
const directiveMeta = typeChecker.getDirectiveMetadata(decl);
9291

9392
if (directiveMeta && directiveMeta.decorator && !directiveMeta.isStandalone) {
9493
let decorator = addStandaloneToDecorator(directiveMeta.decorator);
9594

9695
if (directiveMeta.isComponent) {
97-
const importsToAdd =
98-
getComponentImportExpressions(ref, allDeclarations, tracker, typeChecker, importRemapper);
96+
const importsToAdd = getComponentImportExpressions(
97+
decl, allDeclarations, tracker, typeChecker, importRemapper);
9998

10099
if (importsToAdd.length > 0) {
101100
decorator = addPropertyToAngularDecorator(
@@ -107,7 +106,7 @@ export function convertNgModuleDeclarationToStandalone(
107106

108107
tracker.replaceNode(directiveMeta.decorator, decorator);
109108
} else {
110-
const pipeMeta = typeChecker.getPipeMetadata(ref.node);
109+
const pipeMeta = typeChecker.getPipeMetadata(decl);
111110

112111
if (pipeMeta && pipeMeta.decorator && !pipeMeta.isStandalone) {
113112
tracker.replaceNode(pipeMeta.decorator, addStandaloneToDecorator(pipeMeta.decorator));
@@ -118,26 +117,25 @@ export function convertNgModuleDeclarationToStandalone(
118117
/**
119118
* Gets the expressions that should be added to a component's
120119
* `imports` array based on its template dependencies.
121-
* @param ref Reference to the component class.
120+
* @param decl Component class declaration.
122121
* @param allDeclarations All the declarations that are being converted as a part of this migration.
123122
* @param tracker
124123
* @param typeChecker
125124
* @param importRemapper
126125
*/
127126
function getComponentImportExpressions(
128-
ref: Reference<ts.ClassDeclaration>, allDeclarations: Reference<ts.ClassDeclaration>[],
129-
tracker: ChangeTracker, typeChecker: TemplateTypeChecker,
130-
importRemapper?: ComponentImportsRemapper): ts.Expression[] {
131-
const templateDependencies = findTemplateDependencies(ref, typeChecker);
132-
const usedDependenciesInMigration = new Set(templateDependencies.filter(
133-
dep => allDeclarations.find(current => current.node === dep.node)));
127+
decl: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
128+
typeChecker: TemplateTypeChecker, importRemapper?: ComponentImportsRemapper): ts.Expression[] {
129+
const templateDependencies = findTemplateDependencies(decl, typeChecker);
130+
const usedDependenciesInMigration =
131+
new Set(templateDependencies.filter(dep => allDeclarations.has(dep.node)));
134132
const imports: ts.Expression[] = [];
135133
const seenImports = new Set<string>();
136134
const resolvedDependencies: PotentialImport[] = [];
137135

138136
for (const dep of templateDependencies) {
139137
const importLocation = findImportLocation(
140-
dep as Reference<NamedClassDeclaration>, ref,
138+
dep as Reference<NamedClassDeclaration>, decl,
141139
usedDependenciesInMigration.has(dep) ? PotentialImportMode.ForceDirect :
142140
PotentialImportMode.Normal,
143141
typeChecker);
@@ -149,19 +147,19 @@ function getComponentImportExpressions(
149147
}
150148

151149
const processedDependencies =
152-
importRemapper ? importRemapper(resolvedDependencies, ref) : resolvedDependencies;
150+
importRemapper ? importRemapper(resolvedDependencies, decl) : resolvedDependencies;
153151

154152
for (const importLocation of processedDependencies) {
155153
if (importLocation.moduleSpecifier) {
156154
const identifier = tracker.addImport(
157-
ref.node.getSourceFile(), importLocation.symbolName, importLocation.moduleSpecifier);
155+
decl.getSourceFile(), importLocation.symbolName, importLocation.moduleSpecifier);
158156
imports.push(identifier);
159157
} else {
160158
const identifier = ts.factory.createIdentifier(importLocation.symbolName);
161159

162160
if (importLocation.isForwardReference) {
163161
const forwardRefExpression =
164-
tracker.addImport(ref.node.getSourceFile(), 'forwardRef', '@angular/core');
162+
tracker.addImport(decl.getSourceFile(), 'forwardRef', '@angular/core');
165163
const arrowFunction = ts.factory.createArrowFunction(
166164
undefined, undefined, [], undefined, undefined, identifier);
167165
imports.push(
@@ -184,15 +182,13 @@ function getComponentImportExpressions(
184182
* @param templateTypeChecker
185183
*/
186184
function migrateNgModuleClass(
187-
node: ts.ClassDeclaration, allDeclarations: Reference<ts.ClassDeclaration>[],
188-
tracker: ChangeTracker, typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) {
185+
node: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
186+
typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) {
189187
const decorator = templateTypeChecker.getNgModuleMetadata(node)?.decorator;
190188
const metadata = decorator ? extractMetadataLiteral(decorator) : null;
191189

192190
if (metadata) {
193-
moveDeclarationsToImports(
194-
metadata, allDeclarations.map(decl => decl.node), typeChecker, templateTypeChecker,
195-
tracker);
191+
moveDeclarationsToImports(metadata, allDeclarations, typeChecker, templateTypeChecker, tracker);
196192
}
197193
}
198194

@@ -205,7 +201,7 @@ function migrateNgModuleClass(
205201
* @param tracker
206202
*/
207203
function moveDeclarationsToImports(
208-
literal: ts.ObjectLiteralExpression, allDeclarations: ts.ClassDeclaration[],
204+
literal: ts.ObjectLiteralExpression, allDeclarations: Set<ts.ClassDeclaration>,
209205
typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker,
210206
tracker: ChangeTracker): void {
211207
const declarationsProp = findLiteralProperty(literal, 'declarations');
@@ -347,9 +343,9 @@ function isNamedPropertyAssignment(node: ts.Node): node is ts.PropertyAssignment
347343
* @param typeChecker
348344
*/
349345
function findImportLocation(
350-
target: Reference<NamedClassDeclaration>, inComponent: Reference<ts.ClassDeclaration>,
346+
target: Reference<NamedClassDeclaration>, inComponent: ts.ClassDeclaration,
351347
importMode: PotentialImportMode, typeChecker: TemplateTypeChecker): PotentialImport|null {
352-
const importLocations = typeChecker.getPotentialImportsFor(target, inComponent.node, importMode);
348+
const importLocations = typeChecker.getPotentialImportsFor(target, inComponent, importMode);
353349
let firstSameFileImport: PotentialImport|null = null;
354350
let firstModuleImport: PotentialImport|null = null;
355351

@@ -439,15 +435,14 @@ export function findTestObjectsToMigrate(sourceFile: ts.SourceFile, typeChecker:
439435

440436
/**
441437
* Finds the classes corresponding to dependencies used in a component's template.
442-
* @param ref Component in whose template we're looking for dependencies.
438+
* @param decl Component in whose template we're looking for dependencies.
443439
* @param typeChecker
444440
*/
445-
function findTemplateDependencies(
446-
ref: Reference<ts.ClassDeclaration>,
447-
typeChecker: TemplateTypeChecker): Reference<NamedClassDeclaration>[] {
441+
function findTemplateDependencies(decl: ts.ClassDeclaration, typeChecker: TemplateTypeChecker):
442+
Reference<NamedClassDeclaration>[] {
448443
const results: Reference<NamedClassDeclaration>[] = [];
449-
const usedDirectives = typeChecker.getUsedDirectives(ref.node);
450-
const usedPipes = typeChecker.getUsedPipes(ref.node);
444+
const usedDirectives = typeChecker.getUsedDirectives(decl);
445+
const usedPipes = typeChecker.getUsedPipes(decl);
451446

452447
if (usedDirectives !== null) {
453448
for (const dir of usedDirectives) {
@@ -458,7 +453,7 @@ function findTemplateDependencies(
458453
}
459454

460455
if (usedPipes !== null) {
461-
const potentialPipes = typeChecker.getPotentialPipes(ref.node);
456+
const potentialPipes = typeChecker.getPotentialPipes(decl);
462457

463458
for (const pipe of potentialPipes) {
464459
if (ts.isClassDeclaration(pipe.ref.node) &&
@@ -480,7 +475,7 @@ function findTemplateDependencies(
480475
* @param typeChecker
481476
*/
482477
function filterNonBootstrappedDeclarations(
483-
declarations: Reference<ts.ClassDeclaration>[], ngModule: ts.ClassDeclaration,
478+
declarations: ts.ClassDeclaration[], ngModule: ts.ClassDeclaration,
484479
templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker) {
485480
const metadata = templateTypeChecker.getNgModuleMetadata(ngModule);
486481
const metaLiteral =
@@ -513,7 +508,7 @@ function filterNonBootstrappedDeclarations(
513508
}
514509
}
515510

516-
return declarations.filter(ref => !bootstrappedClasses.has(ref.node));
511+
return declarations.filter(ref => !bootstrappedClasses.has(ref));
517512
}
518513

519514
/**
@@ -523,10 +518,10 @@ function filterNonBootstrappedDeclarations(
523518
*/
524519
export function extractDeclarationsFromModule(
525520
ngModule: ts.ClassDeclaration,
526-
templateTypeChecker: TemplateTypeChecker): Reference<ts.ClassDeclaration>[] {
521+
templateTypeChecker: TemplateTypeChecker): ts.ClassDeclaration[] {
527522
const metadata = templateTypeChecker.getNgModuleMetadata(ngModule);
528-
return metadata ? metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node)) as
529-
Reference<ts.ClassDeclaration>[] :
523+
return metadata ? metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node))
524+
.map(decl => decl.node) as ts.ClassDeclaration[] :
530525
[];
531526
}
532527

@@ -539,12 +534,11 @@ export function extractDeclarationsFromModule(
539534
* @param typeChecker
540535
*/
541536
export function migrateTestDeclarations(
542-
testObjects: ts.ObjectLiteralExpression[],
543-
declarationsOutsideOfTestFiles: Reference<ts.ClassDeclaration>[], tracker: ChangeTracker,
537+
testObjects: Set<ts.ObjectLiteralExpression>,
538+
declarationsOutsideOfTestFiles: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
544539
templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker) {
545540
const {decorators, componentImports} = analyzeTestingModules(testObjects, typeChecker);
546-
const allDeclarations: ts.ClassDeclaration[] =
547-
declarationsOutsideOfTestFiles.map(ref => ref.node);
541+
const allDeclarations = new Set(declarationsOutsideOfTestFiles);
548542

549543
for (const decorator of decorators) {
550544
const closestClass = closestNode(decorator.node, ts.isClassDeclaration);
@@ -553,14 +547,14 @@ export function migrateTestDeclarations(
553547
tracker.replaceNode(decorator.node, addStandaloneToDecorator(decorator.node));
554548

555549
if (closestClass) {
556-
allDeclarations.push(closestClass);
550+
allDeclarations.add(closestClass);
557551
}
558552
} else if (decorator.name === 'Component') {
559553
const newDecorator = addStandaloneToDecorator(decorator.node);
560554
const importsToAdd = componentImports.get(decorator.node);
561555

562556
if (closestClass) {
563-
allDeclarations.push(closestClass);
557+
allDeclarations.add(closestClass);
564558
}
565559

566560
if (importsToAdd && importsToAdd.size > 0) {
@@ -588,7 +582,7 @@ export function migrateTestDeclarations(
588582
* @param testObjects Object literals that should be analyzed.
589583
*/
590584
function analyzeTestingModules(
591-
testObjects: ts.ObjectLiteralExpression[], typeChecker: ts.TypeChecker) {
585+
testObjects: Set<ts.ObjectLiteralExpression>, typeChecker: ts.TypeChecker) {
592586
const seenDeclarations = new Set<ts.Declaration>();
593587
const decorators: NgDecorator[] = [];
594588
const componentImports = new Map<ts.Decorator, Set<ts.Expression>>();
@@ -685,9 +679,9 @@ function extractMetadataLiteral(decorator: ts.Decorator): ts.ObjectLiteralExpres
685679
* @param templateTypeChecker
686680
*/
687681
function isStandaloneDeclaration(
688-
node: ts.ClassDeclaration, declarationsInMigration: ts.ClassDeclaration[],
682+
node: ts.ClassDeclaration, declarationsInMigration: Set<ts.ClassDeclaration>,
689683
templateTypeChecker: TemplateTypeChecker): boolean {
690-
if (declarationsInMigration.includes(node)) {
684+
if (declarationsInMigration.has(node)) {
691685
return true;
692686
}
693687

0 commit comments

Comments
 (0)
0