From 2268278ce99ee70c496d331c71a32eb45f96ba2f Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Tue, 21 Feb 2023 10:57:49 +0100 Subject: [PATCH] fix(migrations): don't copy animations modules into the imports of test components (#49147) Since we have less information about how to copy test components, we copy all the `imports` from the `configureTestingModule` call into the component's `imports`. It fixes some tests, but it can cause issues with animations modules, because they throw errors if they're imported multiple times. These changes add an exception for animations modules imported in testing modules. PR Close #49147 --- .../standalone-bootstrap.ts | 35 +-------- .../standalone-migration/to-standalone.ts | 14 +++- .../ng-generate/standalone-migration/util.ts | 41 ++++++++++ .../test/standalone_migration_spec.ts | 76 +++++++++++++++++++ 4 files changed, 129 insertions(+), 37 deletions(-) diff --git a/packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts b/packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts index 605049fe96d..c9d01e4af32 100644 --- a/packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts +++ b/packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts @@ -16,7 +16,7 @@ import {getAngularDecorators} from '../../utils/ng_decorators'; import {closestNode} from '../../utils/typescript/nodes'; import {ComponentImportsRemapper, convertNgModuleDeclarationToStandalone, extractDeclarationsFromModule, findTestObjectsToMigrate, migrateTestDeclarations} from './to-standalone'; -import {ChangeTracker, findClassDeclaration, findLiteralProperty, getNodeLookup, getRelativeImportPath, ImportRemapper, NamedClassDeclaration, NodeLookup, offsetsToNodes, ReferenceResolver, UniqueItemTracker} from './util'; +import {ChangeTracker, closestOrSelf, findClassDeclaration, findLiteralProperty, getNodeLookup, getRelativeImportPath, ImportRemapper, isClassReferenceInAngularModule, NamedClassDeclaration, NodeLookup, offsetsToNodes, ReferenceResolver, UniqueItemTracker} from './util'; /** Information extracted from a `bootstrapModule` call necessary to migrate it. */ interface BootstrapCallAnalysis { @@ -658,16 +658,6 @@ function isExported(node: ts.Node): node is ts.Node { false; } -/** - * Gets the closest node that matches a predicate, including the node that the search started from. - * @param node Node from which to start the search. - * @param predicate Predicate that the result needs to pass. - */ -function closestOrSelf(node: ts.Node, predicate: (n: ts.Node) => n is T): T| - null { - return predicate(node) ? node : closestNode(node, predicate); -} - /** * Asserts that a node is an exportable declaration, which means that it can either be exported or * it can be safely copied into another file. @@ -680,29 +670,6 @@ function isExportableDeclaration(node: ts.Node): node is ts.EnumDeclaration|ts.C ts.isTypeAliasDeclaration(node); } -/** - * Checks whether a node is referring to a specific class declaration. - * @param node Node that is being checked. - * @param className Name of the class that the node might be referring to. - * @param moduleName Name of the Angular module that should contain the class. - * @param typeChecker - */ -function isClassReferenceInAngularModule( - node: ts.Node, className: string, moduleName: string, typeChecker: ts.TypeChecker): boolean { - const symbol = typeChecker.getTypeAtLocation(node).getSymbol(); - const externalName = `@angular/${moduleName}`; - const internalName = `angular2/rc/packages/${moduleName}`; - - return !!symbol?.declarations?.some(decl => { - const closestClass = closestOrSelf(decl, ts.isClassDeclaration); - const closestClassFileName = closestClass?.getSourceFile().fileName; - return closestClass && closestClassFileName && closestClass.name && - ts.isIdentifier(closestClass.name) && closestClass.name.text === className && - (closestClassFileName.includes(externalName) || - closestClassFileName.includes(internalName)); - }); -} - /** * Gets the index after the last import in a file. Can be used to insert new code into the file. * @param sourceFile File in which to search for imports. 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 ce8e066cf11..64022d9698f 100644 --- a/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts +++ b/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts @@ -15,7 +15,7 @@ import {getImportSpecifier} from '../../utils/typescript/imports'; import {closestNode} from '../../utils/typescript/nodes'; import {isReferenceToImport} from '../../utils/typescript/symbol'; -import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty, ImportRemapper, NamedClassDeclaration} from './util'; +import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty, ImportRemapper, isClassReferenceInAngularModule, NamedClassDeclaration} from './util'; /** * Function that can be used to prcess the dependencies that @@ -602,8 +602,16 @@ function analyzeTestingModules( const importsProp = findLiteralProperty(obj, 'imports'); const importElements = importsProp && hasNgModuleMetadataElements(importsProp) ? - // Filter out calls since they may be a `ModuleWithProviders`. - importsProp.initializer.elements.filter(el => !ts.isCallExpression(el)) : + importsProp.initializer.elements.filter(el => { + // Filter out calls since they may be a `ModuleWithProviders`. + return !ts.isCallExpression(el) && + // Also filter out the animations modules since they throw errors if they're imported + // multiple times and it's common for apps to use the `NoopAnimationsModule` to + // disable animations in screenshot tests. + !isClassReferenceInAngularModule( + el, /^BrowserAnimationsModule|NoopAnimationsModule$/, + 'platform-browser/animations', typeChecker); + }) : null; for (const decl of declarations) { diff --git a/packages/core/schematics/ng-generate/standalone-migration/util.ts b/packages/core/schematics/ng-generate/standalone-migration/util.ts index 0a6284a5bbe..e28a2c15d03 100644 --- a/packages/core/schematics/ng-generate/standalone-migration/util.ts +++ b/packages/core/schematics/ng-generate/standalone-migration/util.ts @@ -12,6 +12,7 @@ import {dirname, relative} from 'path'; import ts from 'typescript'; import {ImportManager} from '../../utils/import_manager'; +import {closestNode} from '../../utils/typescript/nodes'; /** Mapping between a source file and the changes that have to be applied to it. */ export type ChangesByFile = ReadonlyMap; @@ -391,3 +392,43 @@ export function knownInternalAliasRemapper(imports: PotentialImport[]) { {...current, symbolName: 'NgFor'} : current); } + +/** + * Gets the closest node that matches a predicate, including the node that the search started from. + * @param node Node from which to start the search. + * @param predicate Predicate that the result needs to pass. + */ +export function closestOrSelf( + node: ts.Node, predicate: (n: ts.Node) => n is T): T|null { + return predicate(node) ? node : closestNode(node, predicate); +} + +/** + * Checks whether a node is referring to a specific class declaration. + * @param node Node that is being checked. + * @param className Name of the class that the node might be referring to. + * @param moduleName Name of the Angular module that should contain the class. + * @param typeChecker + */ +export function isClassReferenceInAngularModule( + node: ts.Node, className: string|RegExp, moduleName: string, + typeChecker: ts.TypeChecker): boolean { + const symbol = typeChecker.getTypeAtLocation(node).getSymbol(); + const externalName = `@angular/${moduleName}`; + const internalName = `angular2/rc/packages/${moduleName}`; + + return !!symbol?.declarations?.some(decl => { + const closestClass = closestOrSelf(decl, ts.isClassDeclaration); + const closestClassFileName = closestClass?.getSourceFile().fileName; + + if (!closestClass || !closestClassFileName || !closestClass.name || + !ts.isIdentifier(closestClass.name) || + (!closestClassFileName.includes(externalName) && + !closestClassFileName.includes(internalName))) { + return false; + } + + return typeof className === 'string' ? closestClass.name.text === className : + className.test(closestClass.name.text); + }); +} diff --git a/packages/core/schematics/test/standalone_migration_spec.ts b/packages/core/schematics/test/standalone_migration_spec.ts index d3d02be882b..de5eec13601 100644 --- a/packages/core/schematics/test/standalone_migration_spec.ts +++ b/packages/core/schematics/test/standalone_migration_spec.ts @@ -1274,6 +1274,82 @@ describe('standalone migration', () => { `)); }); + it('should not copy over the NoopAnimationsModule into the imports of a test component', + async () => { + writeFile('app.spec.ts', ` + import {NgModule, Component} from '@angular/core'; + import {TestBed} from '@angular/core/testing'; + import {MatCardModule} from '@angular/material/card'; + import {NoopAnimationsModule} from '@angular/platform-browser/animations'; + + describe('bootstrapping an app', () => { + it('should work', () => { + TestBed.configureTestingModule({ + imports: [MatCardModule, NoopAnimationsModule], + declarations: [App] + }); + const fixture = TestBed.createComponent(App); + expect(fixture.nativeElement.innerHTML).toBe('Hello'); + }); + }); + + @Component({template: 'hello'}) + class App {} + `); + + await runMigration('convert-to-standalone'); + + const content = stripWhitespace(tree.readContent('app.spec.ts')); + + expect(content).toContain(stripWhitespace(` + TestBed.configureTestingModule({ + imports: [MatCardModule, NoopAnimationsModule, App] + }); + `)); + expect(content).toContain(stripWhitespace(` + @Component({template: 'hello', standalone: true, imports: [MatCardModule]}) + class App {} + `)); + }); + + it('should not copy over the BrowserAnimationsModule into the imports of a test component', + async () => { + writeFile('app.spec.ts', ` + import {NgModule, Component} from '@angular/core'; + import {TestBed} from '@angular/core/testing'; + import {MatCardModule} from '@angular/material/card'; + import {BrowserAnimationsModule} from '@angular/platform-browser/animations'; + + describe('bootstrapping an app', () => { + it('should work', () => { + TestBed.configureTestingModule({ + imports: [MatCardModule, BrowserAnimationsModule], + declarations: [App] + }); + const fixture = TestBed.createComponent(App); + expect(fixture.nativeElement.innerHTML).toBe('Hello'); + }); + }); + + @Component({template: 'hello'}) + class App {} + `); + + await runMigration('convert-to-standalone'); + + const content = stripWhitespace(tree.readContent('app.spec.ts')); + + expect(content).toContain(stripWhitespace(` + TestBed.configureTestingModule({ + imports: [MatCardModule, BrowserAnimationsModule, App] + }); + `)); + expect(content).toContain(stripWhitespace(` + @Component({template: 'hello', standalone: true, imports: [MatCardModule]}) + class App {} + `)); + }); + it('should not move declarations that are not being migrated out of the declarations array', async () => { const appComponentContent = `