Skip to content

fix(migrations): avoid migrating the same class multiple times in standalone migration #49245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crisbeto wants to merge 1 commit into from
Closed

fix(migrations): avoid migrating the same class multiple times in standalone migration #49245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts
View file
Open in desktop
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
Open in desktop
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));
Copy link
Member Author

@crisbeto crisbeto Feb 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I didn't add a test for this case, because the way the string buffer works from the CLI means that the two conflicting edits of the same class are valid. This issue came up internally where we use a different way of manipulating strings.

}

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