fix(migrations): avoid internal modules when generating imports (#48958)

crisbeto · atscott · commit 32cf4e5cb989 · 2023-02-06T12:27:52.000-08:00
Adds some logic to prefer non-Angular-internal modules when generating imports. This allows us to generate better code for some cases like the `ɵInternalFormsSharedModule` in Forms. Also adds some logic to prefer symbols that are already in the same file. Fixes #48942. PR Close #48958
diff --git a/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts b/packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts
@@ -310,6 +310,7 @@ function findImportLocation(
     target: Reference<NamedClassDeclaration>, inComponent: Reference<ts.ClassDeclaration>,
     importMode: PotentialImportMode, typeChecker: TemplateTypeChecker): PotentialImport|null {
   const importLocations = typeChecker.getPotentialImportsFor(target, inComponent.node, importMode);
+  let firstSameFileImport: PotentialImport|null = null;
   let firstModuleImport: PotentialImport|null = null;
 
   for (const location of importLocations) {
@@ -318,12 +319,17 @@ function findImportLocation(
     if (location.kind === PotentialImportKind.Standalone) {
       return location;
     }
-    if (location.kind === PotentialImportKind.NgModule && !firstModuleImport) {
+    if (!location.moduleSpecifier && !firstSameFileImport) {
+      firstSameFileImport = location;
+    }
+    if (location.kind === PotentialImportKind.NgModule && !firstModuleImport &&
+        // ɵ is used for some internal Angular modules that we want to skip over.
+        !location.symbolName.startsWith('ɵ')) {
       firstModuleImport = location;
     }
   }
 
-  return firstModuleImport;
+  return firstSameFileImport || firstModuleImport || importLocations[0] || null;
 }
 
 /**
diff --git a/packages/core/schematics/test/standalone_migration_spec.ts b/packages/core/schematics/test/standalone_migration_spec.ts
@@ -970,6 +970,52 @@ describe('standalone migration', () => {
     expect(myCompContent).toContain('imports: [ButtonModule]');
   });
 
+  it('should not reference internal modules', async () => {
+    writeFile('./should-migrate/module.ts', `
+      import {NgModule} from '@angular/core';
+      import {MyComp} from './comp';
+      import {ɵButtonModule} from '../do-not-migrate/button.module';
+
+      @NgModule({imports: [ɵButtonModule], declarations: [MyComp]})
+      export class Mod {}
+    `);
+
+    writeFile('./should-migrate/comp.ts', `
+      import {Component} from '@angular/core';
+
+      @Component({selector: 'my-comp', template: '<my-button>Hello</my-button>'})
+      export class MyComp {}
+    `);
+
+    writeFile('./do-not-migrate/button.module.ts', `
+      import {NgModule, forwardRef} from '@angular/core';
+      import {MyButton} from './button';
+
+      @NgModule({
+        imports: [forwardRef(() => ɵButtonModule)],
+        exports: [forwardRef(() => ɵButtonModule)]
+      })
+      export class ExporterModule {}
+
+      @NgModule({declarations: [MyButton], exports: [MyButton]})
+      export class ɵButtonModule {}
+    `);
+
+    writeFile('./do-not-migrate/button.ts', `
+      import {Component} from '@angular/core';
+
+      @Component({selector: 'my-button', template: '<ng-content></ng-content>'})
+      export class MyButton {}
+    `);
+
+    await runMigration('convert-to-standalone', './should-migrate');
+
+    const myCompContent = tree.readContent('./should-migrate/comp.ts');
+    expect(myCompContent)
+        .toContain(`import { ExporterModule } from '../do-not-migrate/button.module';`);
+    expect(myCompContent).toContain('imports: [ExporterModule]');
+  });
+
   it('should migrate tests with a component declared through TestBed', async () => {
     writeFile('app.spec.ts', `
       import {NgModule, Component} from '@angular/core';

Original file line number	Diff line number	Diff line change
`@@ -310,6 +310,7 @@ function findImportLocation(`
`310`	`310`	`target: Reference<NamedClassDeclaration>, inComponent: Reference<ts.ClassDeclaration>,`
`311`	`311`	`importMode: PotentialImportMode, typeChecker: TemplateTypeChecker): PotentialImport\|null {`
`312`	`312`	`const importLocations = typeChecker.getPotentialImportsFor(target, inComponent.node, importMode);`
	`313`	`+ let firstSameFileImport: PotentialImport\|null = null;`
`313`	`314`	`let firstModuleImport: PotentialImport\|null = null;`
`314`	`315`
`315`	`316`	`for (const location of importLocations) {`
`@@ -318,12 +319,17 @@ function findImportLocation(`
`318`	`319`	`if (location.kind === PotentialImportKind.Standalone) {`
`319`	`320`	`return location;`
`320`	`321`	`}`
`321`		`- if (location.kind === PotentialImportKind.NgModule && !firstModuleImport) {`
	`322`	`+ if (!location.moduleSpecifier && !firstSameFileImport) {`
	`323`	`+ firstSameFileImport = location;`
	`324`	`+ }`
	`325`	`+ if (location.kind === PotentialImportKind.NgModule && !firstModuleImport &&`
	`326`	`+ // ɵ is used for some internal Angular modules that we want to skip over.`
	`327`	`+ !location.symbolName.startsWith('ɵ')) {`
`322`	`328`	`firstModuleImport = location;`
`323`	`329`	`}`
`324`	`330`	`}`
`325`	`331`
`326`		`- return firstModuleImport;`
	`332`	`+ return firstSameFileImport \|\| firstModuleImport \|\| importLocations[0] \|\| null;`
`327`	`333`	`}`
`328`	`334`
`329`	`335`	`/**`