Skip to content

Commit 4d23b60

Browse files
devversionAndrewKushnir
authored andcommitted
fix(core): missing-injectable migration should not migrate providers 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
1 parent eeecbf2 commit 4d23b60

File tree

5 files changed

+49
-15
lines changed

5 files changed

+49
-15
lines changed

integration/ng_update_migrations/src/app/migration-tests/providers-test-module.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ export class ComponentProvider2 {}
88
@Component({
99
template: '',
1010
viewProviders: [ComponentTypeProvider, [
11-
{provide: ComponentDontNeedCase, useExisting: ComponentProvider}]
11+
{provide: ComponentDontNeedCase, useClass: ComponentProvider}]
1212
],
1313
providers: [ComponentProvider2]
1414
})
@@ -21,7 +21,7 @@ export class DirectiveProvider {}
2121
@Directive({
2222
selector: 'test-dir',
2323
providers: [DirectiveTypeProvider, [
24-
{provide: DirectiveDontNeedCase, useExisting: DirectiveProvider}]
24+
{provide: DirectiveDontNeedCase, useClass: DirectiveProvider}]
2525
],
2626
})
2727
export class ProvidersTestDirective {}

integration/ng_update_migrations/src/app/migration-tests/providers-test-module_expected.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export class ComponentProvider2 {}
1111
@Component({
1212
template: '',
1313
viewProviders: [ComponentTypeProvider, [
14-
{provide: ComponentDontNeedCase, useExisting: ComponentProvider}]
14+
{provide: ComponentDontNeedCase, useClass: ComponentProvider}]
1515
],
1616
providers: [ComponentProvider2]
1717
})
@@ -26,7 +26,7 @@ export class DirectiveProvider {}
2626
@Directive({
2727
selector: 'test-dir',
2828
providers: [DirectiveTypeProvider, [
29-
{provide: DirectiveDontNeedCase, useExisting: DirectiveProvider}]
29+
{provide: DirectiveDontNeedCase, useClass: DirectiveProvider}]
3030
],
3131
})
3232
export class ProvidersTestDirective {}

packages/core/schematics/migrations/missing-injectable/transform.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -169,12 +169,11 @@ export class MissingInjectableTransform {
169169
if (value instanceof Reference && ts.isClassDeclaration(value.node)) {
170170
this.migrateProviderClass(value.node, module);
171171
} else if (value instanceof Map) {
172-
if (!value.has('provide') || value.has('useValue') || value.has('useFactory')) {
172+
if (!value.has('provide') || value.has('useValue') || value.has('useFactory') ||
173+
value.has('useExisting')) {
173174
return [];
174175
}
175-
if (value.has('useExisting')) {
176-
return this._visitProviderResolvedValue(value.get('useExisting') !, module);
177-
} else if (value.has('useClass')) {
176+
if (value.has('useClass')) {
178177
return this._visitProviderResolvedValue(value.get('useClass') !, module);
179178
} else {
180179
return this._visitProviderResolvedValue(value.get('provide') !, module);

packages/core/schematics/test/google3/missing_injectable_rule_spec.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ describe('Google3 missing injectable tslint rule', () => {
151151
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceB/);
152152
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceC/);
153153
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceD/);
154-
expect(getFile('/index.ts')).toMatch(/@Injectable\(\)\s+export class MyServiceE/);
154+
expect(getFile('/index.ts')).toMatch(/MyServiceD {}\s+export class MyServiceE/);
155155
expect(getFile('/index.ts')).toMatch(/MyServiceE {}\s+export class MyServiceF/);
156156
expect(getFile('/index.ts')).toMatch(/MyServiceF {}\s+export class MyServiceG/);
157157
});

packages/core/schematics/test/missing_injectable_migration_spec.ts

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -189,24 +189,24 @@ describe('Missing injectable migration', () => {
189189
expect(tree.readContent('/index.ts')).not.toContain('@Injectable');
190190
});
191191

192-
it(`should migrate object literal provider with "useExisting" in ${type}`, async() => {
192+
it(`should not migrate object literal provider with "useExisting" in ${type}`, async() => {
193193
writeFile('/index.ts', `
194194
import {${type}} from '@angular/core';
195195
196196
export class MyService {}
197197
export class MyToken {}
198198
199-
@${type}({${propName}: [{provide: MyToken, useExisting: MyService}]})
199+
@${type}({${propName}: [
200+
{provide: MyService: useValue: null},
201+
{provide: MyToken, useExisting: MyService},
202+
]})
200203
export class TestClass {}
201204
`);
202205

203206
await runMigration();
204207

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

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

232+
it(`should not migrate references for providers with "useExisting" in ${type}, but migrate ` +
233+
`existing token if declared in other ${type}`,
234+
async() => {
235+
writeFile('/index.ts', `
236+
import {${type}} from '@angular/core';
237+
238+
export class MyService {}
239+
export class MyToken {}
240+
241+
@${type}({
242+
${propName}: [
243+
{provide: MyToken, useExisting: MyService},
244+
],
245+
})
246+
export class TestClass {}
247+
`);
248+
249+
writeFile('/other.ts', `
250+
import {${type} from '@angular/core';
251+
import {MyService} from './index';
252+
253+
export @${type}({
254+
${propName}: [{provide: MyService}],
255+
})
256+
export class OtherClass {}
257+
`);
258+
259+
await runMigration();
260+
261+
expect(warnOutput.length).toBe(0);
262+
expect(tree.readContent('/index.ts'))
263+
.toMatch(/@angular\/core';\s+@Injectable\(\)\s+export class MyService/);
264+
expect(tree.readContent('/index.ts')).toMatch(/MyService {}\s+export class MyToken/);
265+
});
266+
232267
it('should not migrate provider which is already decorated with @Injectable', async() => {
233268
writeFile('/index.ts', `
234269
import {Injectable, ${type}} from '@angular/core';

0 commit comments

Comments
 (0)