Skip to content

Commit

Permalink
fix(core): missing-injectable migration should not migrate `@NgModule…
Browse files Browse the repository at this point in the history
…` 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
  • Loading branch information
devversion authored and matsko committed Apr 21, 2020
1 parent f186c32 commit 28995db
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
18 changes: 18 additions & 0 deletions packages/core/schematics/test/missing_injectable_migration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down

0 comments on commit 28995db

Please sign in to comment.