Skip to content

Commit

Permalink
fix(core): missing-injectable migration should not migrate providers …
Browse files Browse the repository at this point in the history
…with "useExisting" (#33286)

We should not migrate the reference from `useExisting`. This is because
developers can only use the `useExisting` value as a token. e.g.

```ts
@NgModule({
  providers: [
    {provide: AppRippleConfig, useValue: rippleOptions},
    {provide: MAT_RIPPLE_OPTIONS, useExisting: AppRippleConfig},
  ]
})
export class AppModule {}
```

In the case above, nothing should be decorated with `@Injectable`. The
`AppRippleConfig` class is just used as a token for injection.

PR Close #33286
  • Loading branch information
devversion authored and AndrewKushnir committed Oct 25, 2019
1 parent eeecbf2 commit 4d23b60
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 15 deletions.
Expand Up @@ -8,7 +8,7 @@ export class ComponentProvider2 {}
@Component({
template: '',
viewProviders: [ComponentTypeProvider, [
{provide: ComponentDontNeedCase, useExisting: ComponentProvider}]
{provide: ComponentDontNeedCase, useClass: ComponentProvider}]
],
providers: [ComponentProvider2]
})
Expand All @@ -21,7 +21,7 @@ export class DirectiveProvider {}
@Directive({
selector: 'test-dir',
providers: [DirectiveTypeProvider, [
{provide: DirectiveDontNeedCase, useExisting: DirectiveProvider}]
{provide: DirectiveDontNeedCase, useClass: DirectiveProvider}]
],
})
export class ProvidersTestDirective {}
Expand Down
Expand Up @@ -11,7 +11,7 @@ export class ComponentProvider2 {}
@Component({
template: '',
viewProviders: [ComponentTypeProvider, [
{provide: ComponentDontNeedCase, useExisting: ComponentProvider}]
{provide: ComponentDontNeedCase, useClass: ComponentProvider}]
],
providers: [ComponentProvider2]
})
Expand All @@ -26,7 +26,7 @@ export class DirectiveProvider {}
@Directive({
selector: 'test-dir',
providers: [DirectiveTypeProvider, [
{provide: DirectiveDontNeedCase, useExisting: DirectiveProvider}]
{provide: DirectiveDontNeedCase, useClass: DirectiveProvider}]
],
})
export class ProvidersTestDirective {}
Expand Down
Expand Up @@ -169,12 +169,11 @@ export class MissingInjectableTransform {
if (value instanceof Reference && ts.isClassDeclaration(value.node)) {
this.migrateProviderClass(value.node, module);
} else if (value instanceof Map) {
if (!value.has('provide') || value.has('useValue') || value.has('useFactory')) {
if (!value.has('provide') || value.has('useValue') || value.has('useFactory') ||
value.has('useExisting')) {
return [];
}
if (value.has('useExisting')) {
return this._visitProviderResolvedValue(value.get('useExisting') !, module);
} else if (value.has('useClass')) {
if (value.has('useClass')) {
return this._visitProviderResolvedValue(value.get('useClass') !, module);
} else {
return this._visitProviderResolvedValue(value.get('provide') !, module);
Expand Down
Expand Up @@ -151,7 +151,7 @@ describe('Google3 missing injectable tslint rule', () => {
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/);
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceE/);
expect(getFile('/index.ts')).toMatch(/MyServiceD {}\s+export class MyServiceE/);
expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/);
expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/);
});
Expand Down
47 changes: 41 additions & 6 deletions packages/core/schematics/test/missing_injectable_migration_spec.ts
Expand Up @@ -189,24 +189,24 @@ describe('Missing injectable migration', () => {
expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
});

it(`should migrate object literal provider with "useExisting" in ${type}`, async() => {
it(`should not migrate object literal provider with "useExisting" in ${type}`, async() => {
writeFile('/index.ts', `
import {${type}} from '@angular/core';
export class MyService {}
export class MyToken {}
@${type}({${propName}: [{provide: MyToken, useExisting: MyService}]})
@${type}({${propName}: [
{provide: MyService: useValue: null},
{provide: MyToken, useExisting: MyService},
]})
export class TestClass {}
`);

await runMigration();

expect(warnOutput.length).toBe(0);
expect(tree.readContent('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyService/);
expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/);
expect(tree.readContent('/index.ts'))
.toContain(`{ ${type}, Injectable } from '@angular/core`);
expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
});

it(`should migrate object literal provider with "useClass" in ${type}`, async() => {
Expand All @@ -229,6 +229,41 @@ describe('Missing injectable migration', () => {
.toContain(`{ ${type}, Injectable } from '@angular/core`);
});

it(`should not migrate references for providers with "useExisting" in ${type}, but migrate ` +
`existing token if declared in other ${type}`,
async() => {
writeFile('/index.ts', `
import {${type}} from '@angular/core';
export class MyService {}
export class MyToken {}
@${type}({
${propName}: [
{provide: MyToken, useExisting: MyService},
],
})
export class TestClass {}
`);

writeFile('/other.ts', `
import {${type} from '@angular/core';
import {MyService} from './index';
export @${type}({
${propName}: [{provide: MyService}],
})
export class OtherClass {}
`);

await runMigration();

expect(warnOutput.length).toBe(0);
expect(tree.readContent('/index.ts'))
.toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class MyService/);
expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/);
});

it('should not migrate provider which is already decorated with @Injectable', async() => {
writeFile('/index.ts', `
import {Injectable, ${type}} from '@angular/core';
Expand Down

0 comments on commit 4d23b60

Please sign in to comment.