Skip to content

Commit 2268278

Browse files
crisbetoalxhub
crisbeto
authored and
alxhub
committed
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
1 parent 6966a04 commit 2268278

File tree

4 files changed

+129
-37
lines changed

4 files changed

+129
-37
lines changed

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

Lines changed: 1 addition & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {getAngularDecorators} from '../../utils/ng_decorators';
1616
import {closestNode} from '../../utils/typescript/nodes';
1717

1818
import {ComponentImportsRemapper, convertNgModuleDeclarationToStandalone, extractDeclarationsFromModule, findTestObjectsToMigrate, migrateTestDeclarations} from './to-standalone';
19-
import {ChangeTracker, findClassDeclaration, findLiteralProperty, getNodeLookup, getRelativeImportPath, ImportRemapper, NamedClassDeclaration, NodeLookup, offsetsToNodes, ReferenceResolver, UniqueItemTracker} from './util';
19+
import {ChangeTracker, closestOrSelf, findClassDeclaration, findLiteralProperty, getNodeLookup, getRelativeImportPath, ImportRemapper, isClassReferenceInAngularModule, NamedClassDeclaration, NodeLookup, offsetsToNodes, ReferenceResolver, UniqueItemTracker} from './util';
2020

2121
/** Information extracted from a `bootstrapModule` call necessary to migrate it. */
2222
interface BootstrapCallAnalysis {
@@ -658,16 +658,6 @@ function isExported(node: ts.Node): node is ts.Node {
658658
false;
659659
}
660660

661-
/**
662-
* Gets the closest node that matches a predicate, including the node that the search started from.
663-
* @param node Node from which to start the search.
664-
* @param predicate Predicate that the result needs to pass.
665-
*/
666-
function closestOrSelf<T extends ts.Node>(node: ts.Node, predicate: (n: ts.Node) => n is T): T|
667-
null {
668-
return predicate(node) ? node : closestNode(node, predicate);
669-
}
670-
671661
/**
672662
* Asserts that a node is an exportable declaration, which means that it can either be exported or
673663
* it can be safely copied into another file.
@@ -680,29 +670,6 @@ function isExportableDeclaration(node: ts.Node): node is ts.EnumDeclaration|ts.C
680670
ts.isTypeAliasDeclaration(node);
681671
}
682672

683-
/**
684-
* Checks whether a node is referring to a specific class declaration.
685-
* @param node Node that is being checked.
686-
* @param className Name of the class that the node might be referring to.
687-
* @param moduleName Name of the Angular module that should contain the class.
688-
* @param typeChecker
689-
*/
690-
function isClassReferenceInAngularModule(
691-
node: ts.Node, className: string, moduleName: string, typeChecker: ts.TypeChecker): boolean {
692-
const symbol = typeChecker.getTypeAtLocation(node).getSymbol();
693-
const externalName = `@angular/${moduleName}`;
694-
const internalName = `angular2/rc/packages/${moduleName}`;
695-
696-
return !!symbol?.declarations?.some(decl => {
697-
const closestClass = closestOrSelf(decl, ts.isClassDeclaration);
698-
const closestClassFileName = closestClass?.getSourceFile().fileName;
699-
return closestClass && closestClassFileName && closestClass.name &&
700-
ts.isIdentifier(closestClass.name) && closestClass.name.text === className &&
701-
(closestClassFileName.includes(externalName) ||
702-
closestClassFileName.includes(internalName));
703-
});
704-
}
705-
706673
/**
707674
* Gets the index after the last import in a file. Can be used to insert new code into the file.
708675
* @param sourceFile File in which to search for imports.

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {getImportSpecifier} from '../../utils/typescript/imports';
1515
import {closestNode} from '../../utils/typescript/nodes';
1616
import {isReferenceToImport} from '../../utils/typescript/symbol';
1717

18-
import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty, ImportRemapper, NamedClassDeclaration} from './util';
18+
import {ChangesByFile, ChangeTracker, findClassDeclaration, findLiteralProperty, ImportRemapper, isClassReferenceInAngularModule, NamedClassDeclaration} from './util';
1919

2020
/**
2121
* Function that can be used to prcess the dependencies that
@@ -602,8 +602,16 @@ function analyzeTestingModules(
602602

603603
const importsProp = findLiteralProperty(obj, 'imports');
604604
const importElements = importsProp && hasNgModuleMetadataElements(importsProp) ?
605-
// Filter out calls since they may be a `ModuleWithProviders`.
606-
importsProp.initializer.elements.filter(el => !ts.isCallExpression(el)) :
605+
importsProp.initializer.elements.filter(el => {
606+
// Filter out calls since they may be a `ModuleWithProviders`.
607+
return !ts.isCallExpression(el) &&
608+
// Also filter out the animations modules since they throw errors if they're imported
609+
// multiple times and it's common for apps to use the `NoopAnimationsModule` to
610+
// disable animations in screenshot tests.
611+
!isClassReferenceInAngularModule(
612+
el, /^BrowserAnimationsModule|NoopAnimationsModule$/,
613+
'platform-browser/animations', typeChecker);
614+
}) :
607615
null;
608616

609617
for (const decl of declarations) {

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {dirname, relative} from 'path';
1212
import ts from 'typescript';
1313

1414
import {ImportManager} from '../../utils/import_manager';
15+
import {closestNode} from '../../utils/typescript/nodes';
1516

1617
/** Mapping between a source file and the changes that have to be applied to it. */
1718
export type ChangesByFile = ReadonlyMap<ts.SourceFile, PendingChange[]>;
@@ -391,3 +392,43 @@ export function knownInternalAliasRemapper(imports: PotentialImport[]) {
391392
{...current, symbolName: 'NgFor'} :
392393
current);
393394
}
395+
396+
/**
397+
* Gets the closest node that matches a predicate, including the node that the search started from.
398+
* @param node Node from which to start the search.
399+
* @param predicate Predicate that the result needs to pass.
400+
*/
401+
export function closestOrSelf<T extends ts.Node>(
402+
node: ts.Node, predicate: (n: ts.Node) => n is T): T|null {
403+
return predicate(node) ? node : closestNode(node, predicate);
404+
}
405+
406+
/**
407+
* Checks whether a node is referring to a specific class declaration.
408+
* @param node Node that is being checked.
409+
* @param className Name of the class that the node might be referring to.
410+
* @param moduleName Name of the Angular module that should contain the class.
411+
* @param typeChecker
412+
*/
413+
export function isClassReferenceInAngularModule(
414+
node: ts.Node, className: string|RegExp, moduleName: string,
415+
typeChecker: ts.TypeChecker): boolean {
416+
const symbol = typeChecker.getTypeAtLocation(node).getSymbol();
417+
const externalName = `@angular/${moduleName}`;
418+
const internalName = `angular2/rc/packages/${moduleName}`;
419+
420+
return !!symbol?.declarations?.some(decl => {
421+
const closestClass = closestOrSelf(decl, ts.isClassDeclaration);
422+
const closestClassFileName = closestClass?.getSourceFile().fileName;
423+
424+
if (!closestClass || !closestClassFileName || !closestClass.name ||
425+
!ts.isIdentifier(closestClass.name) ||
426+
(!closestClassFileName.includes(externalName) &&
427+
!closestClassFileName.includes(internalName))) {
428+
return false;
429+
}
430+
431+
return typeof className === 'string' ? closestClass.name.text === className :
432+
className.test(closestClass.name.text);
433+
});
434+
}

‎packages/core/schematics/test/standalone_migration_spec.ts‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1274,6 +1274,82 @@ describe('standalone migration', () => {
12741274
`));
12751275
});
12761276

1277+
it('should not copy over the NoopAnimationsModule into the imports of a test component',
1278+
async () => {
1279+
writeFile('app.spec.ts', `
1280+
import {NgModule, Component} from '@angular/core';
1281+
import {TestBed} from '@angular/core/testing';
1282+
import {MatCardModule} from '@angular/material/card';
1283+
import {NoopAnimationsModule} from '@angular/platform-browser/animations';
1284+
1285+
describe('bootstrapping an app', () => {
1286+
it('should work', () => {
1287+
TestBed.configureTestingModule({
1288+
imports: [MatCardModule, NoopAnimationsModule],
1289+
declarations: [App]
1290+
});
1291+
const fixture = TestBed.createComponent(App);
1292+
expect(fixture.nativeElement.innerHTML).toBe('<hello>Hello</hello>');
1293+
});
1294+
});
1295+
1296+
@Component({template: 'hello'})
1297+
class App {}
1298+
`);
1299+
1300+
await runMigration('convert-to-standalone');
1301+
1302+
const content = stripWhitespace(tree.readContent('app.spec.ts'));
1303+
1304+
expect(content).toContain(stripWhitespace(`
1305+
TestBed.configureTestingModule({
1306+
imports: [MatCardModule, NoopAnimationsModule, App]
1307+
});
1308+
`));
1309+
expect(content).toContain(stripWhitespace(`
1310+
@Component({template: 'hello', standalone: true, imports: [MatCardModule]})
1311+
class App {}
1312+
`));
1313+
});
1314+
1315+
it('should not copy over the BrowserAnimationsModule into the imports of a test component',
1316+
async () => {
1317+
writeFile('app.spec.ts', `
1318+
import {NgModule, Component} from '@angular/core';
1319+
import {TestBed} from '@angular/core/testing';
1320+
import {MatCardModule} from '@angular/material/card';
1321+
import {BrowserAnimationsModule} from '@angular/platform-browser/animations';
1322+
1323+
describe('bootstrapping an app', () => {
1324+
it('should work', () => {
1325+
TestBed.configureTestingModule({
1326+
imports: [MatCardModule, BrowserAnimationsModule],
1327+
declarations: [App]
1328+
});
1329+
const fixture = TestBed.createComponent(App);
1330+
expect(fixture.nativeElement.innerHTML).toBe('<hello>Hello</hello>');
1331+
});
1332+
});
1333+
1334+
@Component({template: 'hello'})
1335+
class App {}
1336+
`);
1337+
1338+
await runMigration('convert-to-standalone');
1339+
1340+
const content = stripWhitespace(tree.readContent('app.spec.ts'));
1341+
1342+
expect(content).toContain(stripWhitespace(`
1343+
TestBed.configureTestingModule({
1344+
imports: [MatCardModule, BrowserAnimationsModule, App]
1345+
});
1346+
`));
1347+
expect(content).toContain(stripWhitespace(`
1348+
@Component({template: 'hello', standalone: true, imports: [MatCardModule]})
1349+
class App {}
1350+
`));
1351+
});
1352+
12771353
it('should not move declarations that are not being migrated out of the declarations array',
12781354
async () => {
12791355
const appComponentContent = `

0 commit comments

Comments
 (0)