Skip to content

Commit

Permalink
fix(migrations): don't copy animations modules into the imports of te…
Browse files Browse the repository at this point in the history 
…st 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
  • Loading branch information
@crisbeto @alxhub
crisbeto authored and alxhub committed Feb 21, 2023
1 parent 6301935 commit c98c6a8
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 37 deletions.
35 changes: 1 addition & 34 deletions packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<T extends ts.Node>(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.
Expand All @@ -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.
Expand Down
14 changes: 11 additions & 3 deletions packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts
View file
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
41 changes: 41 additions & 0 deletions packages/core/schematics/ng-generate/standalone-migration/util.ts
View file
Expand Up @@ -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<ts.SourceFile, PendingChange[]>;
Expand Down Expand Up @@ -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<T extends ts.Node>(
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);
});
}
76 changes: 76 additions & 0 deletions packages/core/schematics/test/standalone_migration_spec.ts
View file
Expand Up @@ -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>Hello</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>Hello</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 = `
Expand Down

0 comments on commit c98c6a8

Please sign in to comment.