From 28995dba19b43160249a15b2bec879aa82314b95 Mon Sep 17 00:00:00 2001
From: Paul Gschwendtner <paulgschwendtner@gmail.com>
Date: Mon, 6 Apr 2020 15:04:48 +0200
Subject: [PATCH] fix(core): missing-injectable migration should not migrate
 `@NgModule` classes (#36369)

Based on the migration guide, provided classes which don't have
either `@Injectable`, `@Directive`, `@Component` or `@Pipe` need
to be migrated.

This is not correct as provided classes with an `@NgModule` also
have a factory function that can be read by the r3 injector. It's
unclear in which cases the `@NgModule` decorator is used for
provided classes, but this scenario has been reported.

Either we fix this in the migration, or we make sure to report
this as unsupported in the Ivy compiler.

Fixes #35700.

PR Close #36369
---
 .../migrations/missing-injectable/transform.ts | 12 ++++++++----
 .../test/missing_injectable_migration_spec.ts  | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/packages/core/schematics/migrations/missing-injectable/transform.ts b/packages/core/schematics/migrations/missing-injectable/transform.ts
index aad21c9d12..f49f34b120 100644
--- a/packages/core/schematics/migrations/missing-injectable/transform.ts
+++ b/packages/core/schematics/migrations/missing-injectable/transform.ts
@@ -18,10 +18,14 @@ import {ResolvedDirective, ResolvedNgModule} from './definition_collector';
 import {ProviderLiteral, ProvidersEvaluator} from './providers_evaluator';
 import {UpdateRecorder} from './update_recorder';
 
-
-
-/** Name of decorators which imply that a given class does not need to be migrated. */
-const NO_MIGRATE_DECORATORS = ['Injectable', 'Directive', 'Component', 'Pipe'];
+/**
+ * Name of decorators which imply that a given class does not need to be migrated.
+ *    - `@Injectable`, `@Directive`, `@Component` and `@Pipe` instruct the compiler
+ *       to generate a factory definition.
+ *    - `@NgModule` instructs the compiler to generate a provider definition that holds
+ *       the factory function.
+ */
+const NO_MIGRATE_DECORATORS = ['Injectable', 'Directive', 'Component', 'Pipe', 'NgModule'];
 
 export interface AnalysisFailure {
   node: ts.Node;
diff --git a/packages/core/schematics/test/missing_injectable_migration_spec.ts b/packages/core/schematics/test/missing_injectable_migration_spec.ts
index f07027a538..28e6787710 100644
--- a/packages/core/schematics/test/missing_injectable_migration_spec.ts
+++ b/packages/core/schematics/test/missing_injectable_migration_spec.ts
@@ -350,6 +350,24 @@ describe('Missing injectable migration', () => {
       expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
     });
 
+    it('should not migrate provider which is already decorated with @NgModule', async () => {
+      const importedSymbols = type !== 'NgModule' ? ['NgModule', type] : ['NgModule'];
+      writeFile('/index.ts', `
+        import {${importedSymbols.join(', ')}} from '@angular/core';
+
+        @NgModule()
+        export class MyOtherModule {}
+
+        @${type}({${propName}: [MyOtherModule]})
+        export class TestClass {}
+      `);
+
+      await runMigration();
+
+      expect(warnOutput.length).toBe(0);
+      expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
+    });
+
     it(`should migrate multiple providers in same ${type}`, async () => {
       writeFile('/index.ts', `
       import {${type}} from '@angular/core';