From 6377487b1ab7679cef9a44f88440fe5e8eb97480 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 7 Feb 2023 10:10:28 +0100 Subject: [PATCH] fix(migrations): only exclude bootstrapped declarations from initial standalone migration (#48987) Currently the standalone migration is set up to skip any modules that have a `bootstrap` array with at least one element. This ends up being misleading for small apps who have everything in the root module. These changes add some logic to only skip the root component. Fixes #48944. PR Close #48987 --- .../standalone-migration/to-standalone.ts | 86 ++++++++++++++----- .../test/standalone_migration_spec.ts | 40 +++++++++ 2 files changed, 104 insertions(+), 22 deletions(-) diff --git a/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts b/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts index 181fcba8193..a607ff2f40d 100644 --- a/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts +++ b/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts @@ -35,9 +35,17 @@ export function toStandalone( for (const sourceFile of sourceFiles) { const {modules, testObjects} = findModulesToMigrate(sourceFile, typeChecker); - modules.forEach( - module => declarations.push(...extractDeclarationsFromModule(module, templateTypeChecker))); - modulesToMigrate.push(...modules); + + for (const module of modules) { + const moduleDeclarations = + extractDeclarationsFromModule(module, typeChecker, templateTypeChecker); + + if (moduleDeclarations.length > 0) { + modulesToMigrate.push(module); + declarations.push(...moduleDeclarations); + } + } + testObjectsToMigrate.push(...testObjects); } @@ -153,13 +161,11 @@ function migrateNgModuleClass( node: ts.ClassDeclaration, allDeclarations: Reference[], tracker: ChangeTracker, typeChecker: ts.TypeChecker, templateTypeChecker: TemplateTypeChecker) { const decorator = templateTypeChecker.getNgModuleMetadata(node)?.decorator; + const metadata = decorator ? extractMetadataLiteral(decorator) : null; - if (decorator && ts.isCallExpression(decorator.expression) && - decorator.expression.arguments.length === 1 && - ts.isObjectLiteralExpression(decorator.expression.arguments[0])) { + if (metadata) { moveDeclarationsToImports( - decorator.expression.arguments[0], allDeclarations.map(decl => decl.node), typeChecker, - tracker); + metadata, allDeclarations.map(decl => decl.node), typeChecker, tracker); } } @@ -355,18 +361,12 @@ function findModulesToMigrate(sourceFile: ts.SourceFile, typeChecker: ts.TypeChe if (ts.isClassDeclaration(node)) { const decorator = getAngularDecorators(typeChecker, ts.getDecorators(node) || []) .find(current => current.name === 'NgModule'); - const metadata = decorator?.node.expression.arguments[0]; + const metadata = decorator ? extractMetadataLiteral(decorator.node) : null; - if (metadata && ts.isObjectLiteralExpression(metadata)) { + if (metadata) { const declarations = findLiteralProperty(metadata, 'declarations'); - const bootstrap = findLiteralProperty(metadata, 'bootstrap'); - const hasDeclarations = declarations != null && hasNgModuleMetadataElements(declarations); - const hasBootstrap = bootstrap != null && hasNgModuleMetadataElements(bootstrap); - // Skip modules that bootstrap components since changing them would also involve - // converting `bootstrapModule` calls to `bootstrapApplication`. These declarations - // will be converted to standalone in the `standalone-bootstrap` step. - if (hasDeclarations && !hasBootstrap) { + if (declarations != null && hasNgModuleMetadataElements(declarations)) { modules.push(node); } } @@ -424,11 +424,53 @@ function findTemplateDependencies( /** Extracts classes that are referred to in a module's `declarations` array. */ function extractDeclarationsFromModule( - ngModule: ts.ClassDeclaration, - typeChecker: TemplateTypeChecker): Reference[] { - return typeChecker.getNgModuleMetadata(ngModule)?.declarations.filter( - decl => ts.isClassDeclaration(decl.node)) as Reference[] || - []; + ngModule: ts.ClassDeclaration, typeChecker: ts.TypeChecker, + templateTypeChecker: TemplateTypeChecker): Reference[] { + const metadata = templateTypeChecker.getNgModuleMetadata(ngModule); + + if (!metadata) { + return []; + } + + const allDeclarations = metadata.declarations.filter(decl => ts.isClassDeclaration(decl.node)) as + Reference[]; + const metaLiteral = metadata.decorator ? extractMetadataLiteral(metadata.decorator) : null; + const bootstrapProp = metaLiteral ? findLiteralProperty(metaLiteral, 'bootstrap') : null; + + // If there's no `bootstrap`, we can migrate all the declarations. + if (!bootstrapProp) { + return allDeclarations; + } + + // If we can't analyze the `bootstrap` property, we can't safely migrate the module. + if (!ts.isPropertyAssignment(bootstrapProp) || + !ts.isArrayLiteralExpression(bootstrapProp.initializer)) { + return []; + } + + const bootstrappedClasses = new Set(); + + for (const el of bootstrapProp.initializer.elements) { + const referencedClass = ts.isIdentifier(el) ? findClassDeclaration(el, typeChecker) : null; + + // If we manage to resolve the class, we can use it to filter out declarations that are + // being bootstrapped. Otherwise we risk breaking the app so we skip the entire module. + if (referencedClass) { + bootstrappedClasses.add(referencedClass); + } else { + return []; + } + } + + return allDeclarations.filter(ref => !bootstrappedClasses.has(ref.node)); +} + +/** Extracts the metadata object literal from an Angular decorator. */ +function extractMetadataLiteral(decorator: ts.Decorator): ts.ObjectLiteralExpression|null { + return ts.isCallExpression(decorator.expression) && decorator.expression.arguments.length === 1 && + ts.isObjectLiteralExpression(decorator.expression.arguments[0]) ? + decorator.expression.arguments[0] : + null; } /** diff --git a/packages/core/schematics/test/standalone_migration_spec.ts b/packages/core/schematics/test/standalone_migration_spec.ts index 88ec9cf8685..d781baf2715 100644 --- a/packages/core/schematics/test/standalone_migration_spec.ts +++ b/packages/core/schematics/test/standalone_migration_spec.ts @@ -1306,6 +1306,46 @@ describe('standalone migration', () => { `)); }); + it('should migrate declarations that are not being bootstrapped in a root module', async () => { + writeFile('dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[foo]'}) + export class MyDir {} + `); + + writeFile('module.ts', ` + import {NgModule, Component} from '@angular/core'; + import {MyDir} from './dir'; + + @Component({selector: 'root-comp', template: 'hello'}) + export class RootComp {} + + @NgModule({declarations: [RootComp, MyDir], bootstrap: [RootComp]}) + export class Mod {} + `); + + await runMigration('convert-to-standalone'); + + expect(stripWhitespace(tree.readContent('dir.ts'))).toBe(stripWhitespace(` + import {Directive} from '@angular/core'; + + @Directive({selector: '[foo]', standalone: true}) + export class MyDir {} + `)); + + expect(stripWhitespace(tree.readContent('module.ts'))).toBe(stripWhitespace(` + import {NgModule, Component} from '@angular/core'; + import {MyDir} from './dir'; + + @Component({selector: 'root-comp', template: 'hello'}) + export class RootComp {} + + @NgModule({imports: [MyDir], declarations: [RootComp], bootstrap: [RootComp]}) + export class Mod {} + `)); + }); + it('should generate a forwardRef for forward reference within the same file', async () => { writeFile('decls.ts', ` import {Component, Directive} from '@angular/core';