Skip to content

Commit

Permalink
fix(migrations): avoid migrating the same class multiple times in sta…
Browse files Browse the repository at this point in the history 
…ndalone 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
  • Loading branch information
@crisbeto @AndrewKushnir
crisbeto authored and AndrewKushnir committed Feb 28, 2023
1 parent 600fd12 commit 44d095a
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 60 deletions.
10 changes: 5 additions & 5 deletions packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ interface BootstrapCallAnalysis {
/** Component that the module is bootstrapping. */
component: NamedClassDeclaration;
/** Classes declared by the bootstrapped module. */
declarations: Reference<ts.ClassDeclaration>[];
declarations: ts.ClassDeclaration[];
}

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

for (const sourceFile of sourceFiles) {
sourceFile.forEachChild(function walk(node) {
Expand All @@ -59,11 +59,11 @@ export function toStandaloneBootstrap(
node.forEachChild(walk);
});

testObjects.push(...findTestObjectsToMigrate(sourceFile, typeChecker));
findTestObjectsToMigrate(sourceFile, typeChecker).forEach(obj => testObjects.add(obj));
}

for (const call of bootstrapCalls) {
allDeclarations.push(...call.declarations);
call.declarations.forEach(decl => allDeclarations.add(decl));
migrateBootstrapCall(call, tracker, referenceResolver, typeChecker, printer);
}

Expand Down
104 changes: 49 additions & 55 deletions packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty,
* are going to be added to the imports of a component.
*/
export type ComponentImportsRemapper =
(imports: PotentialImport[], component: Reference<ts.ClassDeclaration>) => PotentialImport[];
(imports: PotentialImport[], component: ts.ClassDeclaration) => PotentialImport[];

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

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

if (unbootstrappedDeclarations.length > 0) {
modulesToMigrate.push(module);
declarations.push(...unbootstrappedDeclarations);
modulesToMigrate.add(module);
unbootstrappedDeclarations.forEach(decl => declarations.add(decl));
}
}

testObjectsToMigrate.push(...testObjects);
testObjects.forEach(obj => testObjectsToMigrate.add(obj));
}

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

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

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

if (directiveMeta.isComponent) {
const importsToAdd =
getComponentImportExpressions(ref, allDeclarations, tracker, typeChecker, importRemapper);
const importsToAdd = getComponentImportExpressions(
decl, allDeclarations, tracker, typeChecker, importRemapper);

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

tracker.replaceNode(directiveMeta.decorator, decorator);
} else {
const pipeMeta = typeChecker.getPipeMetadata(ref.node);
const pipeMeta = typeChecker.getPipeMetadata(decl);

if (pipeMeta && pipeMeta.decorator && !pipeMeta.isStandalone) {
tracker.replaceNode(pipeMeta.decorator, addStandaloneToDecorator(pipeMeta.decorator));
Expand All @@ -118,26 +117,25 @@ export function convertNgModuleDeclarationToStandalone(
/**
* Gets the expressions that should be added to a component's
* `imports` array based on its template dependencies.
* @param ref Reference to the component class.
* @param decl Component class declaration.
* @param allDeclarations All the declarations that are being converted as a part of this migration.
* @param tracker
* @param typeChecker
* @param importRemapper
*/
function getComponentImportExpressions(
ref: Reference<ts.ClassDeclaration>, allDeclarations: Reference<ts.ClassDeclaration>[],
tracker: ChangeTracker, typeChecker: TemplateTypeChecker,
importRemapper?: ComponentImportsRemapper): ts.Expression[] {
const templateDependencies = findTemplateDependencies(ref, typeChecker);
const usedDependenciesInMigration = new Set(templateDependencies.filter(
dep => allDeclarations.find(current => current.node === dep.node)));
decl: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
typeChecker: TemplateTypeChecker, importRemapper?: ComponentImportsRemapper): ts.Expression[] {
const templateDependencies = findTemplateDependencies(decl, typeChecker);
const usedDependenciesInMigration =
new Set(templateDependencies.filter(dep => allDeclarations.has(dep.node)));
const imports: ts.Expression[] = [];
const seenImports = new Set<string>();
const resolvedDependencies: PotentialImport[] = [];

for (const dep of templateDependencies) {
const importLocation = findImportLocation(
dep as Reference<NamedClassDeclaration>, ref,
dep as Reference<NamedClassDeclaration>, decl,
usedDependenciesInMigration.has(dep) ? PotentialImportMode.ForceDirect :
PotentialImportMode.Normal,
typeChecker);
Expand All @@ -149,19 +147,19 @@ function getComponentImportExpressions(
}

const processedDependencies =
importRemapper ? importRemapper(resolvedDependencies, ref) : resolvedDependencies;
importRemapper ? importRemapper(resolvedDependencies, decl) : resolvedDependencies;

for (const importLocation of processedDependencies) {
if (importLocation.moduleSpecifier) {
const identifier = tracker.addImport(
ref.node.getSourceFile(), importLocation.symbolName, importLocation.moduleSpecifier);
decl.getSourceFile(), importLocation.symbolName, importLocation.moduleSpecifier);
imports.push(identifier);
} else {
const identifier = ts.factory.createIdentifier(importLocation.symbolName);

if (importLocation.isForwardReference) {
const forwardRefExpression =
tracker.addImport(ref.node.getSourceFile(), 'forwardRef', '@angular/core');
tracker.addImport(decl.getSourceFile(), 'forwardRef', '@angular/core');
const arrowFunction = ts.factory.createArrowFunction(
undefined, undefined, [], undefined, undefined, identifier);
imports.push(
Expand All @@ -184,15 +182,13 @@ function getComponentImportExpressions(
* @param templateTypeChecker
*/
function migrateNgModuleClass(
node: ts.ClassDeclaration, allDeclarations: Reference<ts.ClassDeclaration>[],
tracker: ChangeTracker, typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) {
node: ts.ClassDeclaration, allDeclarations: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) {
const decorator = templateTypeChecker.getNgModuleMetadata(node)?.decorator;
const metadata = decorator ? extractMetadataLiteral(decorator) : null;

if (metadata) {
moveDeclarationsToImports(
metadata, allDeclarations.map(decl => decl.node), typeChecker, templateTypeChecker,
tracker);
moveDeclarationsToImports(metadata, allDeclarations, typeChecker, templateTypeChecker, tracker);
}
}

Expand All @@ -205,7 +201,7 @@ function migrateNgModuleClass(
* @param tracker
*/
function moveDeclarationsToImports(
literal: ts.ObjectLiteralExpression, allDeclarations: ts.ClassDeclaration[],
literal: ts.ObjectLiteralExpression, allDeclarations: Set<ts.ClassDeclaration>,
typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker,
tracker: ChangeTracker): void {
const declarationsProp = findLiteralProperty(literal, 'declarations');
Expand Down Expand Up @@ -347,9 +343,9 @@ function isNamedPropertyAssignment(node: ts.Node): node is ts.PropertyAssignment
* @param typeChecker
*/
function findImportLocation(
target: Reference<NamedClassDeclaration>, inComponent: Reference<ts.ClassDeclaration>,
target: Reference<NamedClassDeclaration>, inComponent: ts.ClassDeclaration,
importMode: PotentialImportMode, typeChecker: TemplateTypeChecker): PotentialImport|null {
const importLocations = typeChecker.getPotentialImportsFor(target, inComponent.node, importMode);
const importLocations = typeChecker.getPotentialImportsFor(target, inComponent, importMode);
let firstSameFileImport: PotentialImport|null = null;
let firstModuleImport: PotentialImport|null = null;

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

/**
* Finds the classes corresponding to dependencies used in a component's template.
* @param ref Component in whose template we're looking for dependencies.
* @param decl Component in whose template we're looking for dependencies.
* @param typeChecker
*/
function findTemplateDependencies(
ref: Reference<ts.ClassDeclaration>,
typeChecker: TemplateTypeChecker): Reference<NamedClassDeclaration>[] {
function findTemplateDependencies(decl: ts.ClassDeclaration, typeChecker: TemplateTypeChecker):
Reference<NamedClassDeclaration>[] {
const results: Reference<NamedClassDeclaration>[] = [];
const usedDirectives = typeChecker.getUsedDirectives(ref.node);
const usedPipes = typeChecker.getUsedPipes(ref.node);
const usedDirectives = typeChecker.getUsedDirectives(decl);
const usedPipes = typeChecker.getUsedPipes(decl);

if (usedDirectives !== null) {
for (const dir of usedDirectives) {
Expand All @@ -458,7 +453,7 @@ function findTemplateDependencies(
}

if (usedPipes !== null) {
const potentialPipes = typeChecker.getPotentialPipes(ref.node);
const potentialPipes = typeChecker.getPotentialPipes(decl);

for (const pipe of potentialPipes) {
if (ts.isClassDeclaration(pipe.ref.node) &&
Expand All @@ -480,7 +475,7 @@ function findTemplateDependencies(
* @param typeChecker
*/
function filterNonBootstrappedDeclarations(
declarations: Reference<ts.ClassDeclaration>[], ngModule: ts.ClassDeclaration,
declarations: ts.ClassDeclaration[], ngModule: ts.ClassDeclaration,
templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker) {
const metadata = templateTypeChecker.getNgModuleMetadata(ngModule);
const metaLiteral =
Expand Down Expand Up @@ -513,7 +508,7 @@ function filterNonBootstrappedDeclarations(
}
}

return declarations.filter(ref => !bootstrappedClasses.has(ref.node));
return declarations.filter(ref => !bootstrappedClasses.has(ref));
}

/**
Expand All @@ -523,10 +518,10 @@ function filterNonBootstrappedDeclarations(
*/
export function extractDeclarationsFromModule(
ngModule: ts.ClassDeclaration,
templateTypeChecker: TemplateTypeChecker): Reference<ts.ClassDeclaration>[] {
templateTypeChecker: TemplateTypeChecker): ts.ClassDeclaration[] {
const metadata = templateTypeChecker.getNgModuleMetadata(ngModule);
return metadata ? metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node)) as
Reference<ts.ClassDeclaration>[] :
return metadata ? metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node))
.map(decl => decl.node) as ts.ClassDeclaration[] :
[];
}

Expand All @@ -539,12 +534,11 @@ export function extractDeclarationsFromModule(
* @param typeChecker
*/
export function migrateTestDeclarations(
testObjects: ts.ObjectLiteralExpression[],
declarationsOutsideOfTestFiles: Reference<ts.ClassDeclaration>[], tracker: ChangeTracker,
testObjects: Set<ts.ObjectLiteralExpression>,
declarationsOutsideOfTestFiles: Set<ts.ClassDeclaration>, tracker: ChangeTracker,
templateTypeChecker: TemplateTypeChecker, typeChecker: ts.TypeChecker) {
const {decorators, componentImports} = analyzeTestingModules(testObjects, typeChecker);
const allDeclarations: ts.ClassDeclaration[] =
declarationsOutsideOfTestFiles.map(ref => ref.node);
const allDeclarations = new Set(declarationsOutsideOfTestFiles);

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

if (closestClass) {
allDeclarations.push(closestClass);
allDeclarations.add(closestClass);
}
} else if (decorator.name === 'Component') {
const newDecorator = addStandaloneToDecorator(decorator.node);
const importsToAdd = componentImports.get(decorator.node);

if (closestClass) {
allDeclarations.push(closestClass);
allDeclarations.add(closestClass);
}

if (importsToAdd && importsToAdd.size > 0) {
Expand Down Expand Up @@ -588,7 +582,7 @@ export function migrateTestDeclarations(
* @param testObjects Object literals that should be analyzed.
*/
function analyzeTestingModules(
testObjects: ts.ObjectLiteralExpression[], typeChecker: ts.TypeChecker) {
testObjects: Set<ts.ObjectLiteralExpression>, typeChecker: ts.TypeChecker) {
const seenDeclarations = new Set<ts.Declaration>();
const decorators: NgDecorator[] = [];
const componentImports = new Map<ts.Decorator, Set<ts.Expression>>();
Expand Down Expand Up @@ -685,9 +679,9 @@ function extractMetadataLiteral(decorator: ts.Decorator): ts.ObjectLiteralExpres
* @param templateTypeChecker
*/
function isStandaloneDeclaration(
node: ts.ClassDeclaration, declarationsInMigration: ts.ClassDeclaration[],
node: ts.ClassDeclaration, declarationsInMigration: Set<ts.ClassDeclaration>,
templateTypeChecker: TemplateTypeChecker): boolean {
if (declarationsInMigration.includes(node)) {
if (declarationsInMigration.has(node)) {
return true;
}

Expand Down

0 comments on commit 44d095a

Please sign in to comment.