Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(di): add better type information to injector.get() #13785

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@mhevery
Copy link
Member

commented Jan 4, 2017

fix(di): add better type information to injector.get()

BREAKING CHANGE:

  • injector.get() is now parameterize and returns a type based on args.
    • injector.get(Foo) returns an instance of Foo
    • injector.get(new OpaqueToken<Foo>(‘Foo’)) returns an instance of
      Foo.
  • OpaqueToken is now parameterized.

Migration

  • If you declare your own OpaqueToken, it must now be parameterized.
  • In the past it was possible that the injector token could return a
    type other than itself. For example
    var cars: Car[] = injector.get(Car) because the provider for Car
    was declared multi: true. This is now no longer possible and it is
    considered a bad form. Proper way to do this is through
    var CARS = new OpaqueToken<Car[]>; and then
    var cars = injector.get(CARS).
  • var foo: any = injector.get(‘something’) is still supported and
    results in any type, but it’s use is strongly discouraged.

@googlebot googlebot added the cla: yes label Jan 4, 2017

@mhevery mhevery force-pushed the mhevery:injector-get branch 4 times, most recently from eea2e36 to 19089f5 Jan 4, 2017

@mhevery mhevery requested review from vicb and IgorMinar Jan 5, 2017

@@ -6,12 +6,13 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Inject, PACKAGE_ROOT_URL} from '@angular/core';
import {Inject, OpaqueToken, PACKAGE_ROOT_URL} from '@angular/core';

This comment has been minimized.

Copy link
@vicb

vicb Jan 5, 2017

Contributor

Why ?

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 6, 2017

Author Member

Because the .d.ts file now has PACKAGE_ROOT_URL from any to OpaqueToken. So the import is needed to make .d.ts happy, even thought OpaqueToken is not used in this file directly (only through inference)

@@ -28,7 +28,7 @@
* error messages.
* @stable
*/
export class OpaqueToken {
export class OpaqueToken<T> {

This comment has been minimized.

Copy link
@vicb

vicb Jan 5, 2017

Contributor

add docs for T

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 6, 2017

Author Member

good point, added.

@@ -53,10 +54,11 @@ export const PLATFORM_INITIALIZER: any = new OpaqueToken('Platform Initializer')
*
* @experimental
*/
export const APP_BOOTSTRAP_LISTENER = new OpaqueToken('appBootstrapListener');
export const APP_BOOTSTRAP_LISTENER =
new OpaqueToken<((compRef: ComponentRef<any>) => void)[]>('appBootstrapListener');

This comment has been minimized.

Copy link
@vicb

vicb Jan 5, 2017

Contributor

should we use _, __, ___, ... for parameter names at this location ?

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 6, 2017

Author Member

I think names should have a meaning. Would like to keep as is.

@@ -28,7 +29,7 @@ function isEmptyInputValue(value: any) {
* {@example core/forms/ts/ng_validators/ng_validators.ts region='ng_validators'}
* @stable
*/
export const NG_VALIDATORS: OpaqueToken = new OpaqueToken('NgValidators');
export const NG_VALIDATORS = new OpaqueToken<Array<Validator|Function>>('NgValidators');

This comment has been minimized.

Copy link
@vicb

vicb Jan 5, 2017

Contributor

Array<> => []
should be a tslint rule probably

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 6, 2017

Author Member

But that would be (Validator|Function)[] which to me looks strange (@chuckjaz agrees).

@@ -42,8 +42,8 @@ export function main() {

http = injector.get(Http);
jsonp = injector.get(Jsonp);
jsonpBackend = injector.get(JSONPBackend);
xhrBackend = injector.get(XHRBackend);
jsonpBackend = injector.get(JSONPBackend) as MockBackend;

This comment has been minimized.

Copy link
@vicb

vicb Jan 5, 2017

Contributor

why is this needed ?

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 6, 2017

Author Member

because jsonBackend is declared as MockBackend. injector.get(JSONPBackend) returns type JSONPBackend which is a subtype of MockBackend hence cast is required.

@mhevery mhevery force-pushed the mhevery:injector-get branch from 19089f5 to 5d77736 Jan 6, 2017

@@ -43,7 +44,7 @@ function _randomChar(): string {
* A function that will be executed when a platform is initialized.
* @experimental
*/
export const PLATFORM_INITIALIZER: any = new OpaqueToken('Platform Initializer');
export const PLATFORM_INITIALIZER = new OpaqueToken<(() => void)[]>('Platform Initializer');

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Jan 6, 2017

Member

Consider changing the type to OpaqueToken<Array<() => void>>.

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 12, 2017

Author Member

fixed

@@ -53,10 +54,11 @@ export const PLATFORM_INITIALIZER: any = new OpaqueToken('Platform Initializer')
*
* @experimental
*/
export const APP_BOOTSTRAP_LISTENER = new OpaqueToken('appBootstrapListener');
export const APP_BOOTSTRAP_LISTENER =
new OpaqueToken<((compRef: ComponentRef<any>) => void)[]>('appBootstrapListener');

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Jan 6, 2017

Member

Consider changing the type to OpaqueToken<Array<(compRef: ComponentRef<any>) => void>>.

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 12, 2017

Author Member

fixed

@IgorMinar
Copy link
Member

left a comment

As raised by others this looks like a bigger breaking change that would have a significant impact on the ecosystem. Our promise was that we are not going to break existing components on npm.

Can we evaluate and document the impact of this change on the ecosystem (existing 3rd party components, applications, tooling, etc) before we merge this in?

@mhevery mhevery force-pushed the mhevery:injector-get branch 6 times, most recently from 4d95a76 to dce3247 Jan 12, 2017

@mhevery

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2017

@IgorMinar / @vicb Could you have a look? This should be a lot less breaking now.

@@ -9,7 +9,7 @@
// Must be imported first, because angular2 decorators throws on load.
import 'reflect-metadata';

export {Injector, OpaqueToken, Provider, ReflectiveInjector} from '@angular/core';
export {InjectionToken, Injector, Provider, ReflectiveInjector} from '@angular/core';

This comment has been minimized.

Copy link
@vicb

vicb Jan 13, 2017

Contributor

Why not Token (or InjectionProvider, ...)

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 13, 2017

Author Member

discussed in person

@@ -507,8 +507,8 @@ export class StaticReflector implements ReflectorReader {
staticSymbol = simplifyInContext(context, expression['expression'], depth + 1);
if (staticSymbol instanceof StaticSymbol) {
if (staticSymbol === self.opaqueToken) {

This comment has been minimized.

Copy link
@vicb

vicb Jan 13, 2017

Contributor

rename opaqueToken ? (grep the whole project)

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 13, 2017

Author Member

replaced

* ### Example ([live demo](http://plnkr.co/edit/Ys9ezXpj2Mnoy3Uc8KBp?p=preview))
*
* ```typescript
* var t = new OpaqueToken("value");

This comment has been minimized.

Copy link
@vicb

vicb Jan 13, 2017

Contributor

update

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 13, 2017

Author Member

this is for old OpaqueToken should be left as is.

@mhevery mhevery force-pushed the mhevery:injector-get branch from dff89a0 to abb52de Jan 13, 2017

@IgorMinar
Copy link
Member

left a comment

please fix the docs. otherwise LGTM!

*
* Using an `OpaqueToken` is preferable to using an `Object` as tokens because it provides better
* error messages.
* @deprecated since v4.0.0 use `InjectionToken<?>` instead.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 13, 2017

Member

can you explain why? (can't carry type info)

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 13, 2017

Author Member

fixed

* {provide: t, useValue: "bindingValue"}
* ]);
*
* expect(injector.get(t)).toEqual("bindingValue");

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Jan 13, 2017

Member

please create this as a standalone example and then use @example to include it here. we've done this for other apis already. Please no more new inline code snippets.

This comment has been minimized.

Copy link
@mhevery

mhevery Jan 13, 2017

Author Member

done

@mhevery mhevery force-pushed the mhevery:injector-get branch 2 times, most recently from 12f97ff to 66a6f96 Jan 13, 2017

@mhevery mhevery modified the milestone: merg Jan 17, 2017

feat(core): Add type information to injector.get()
- Introduce `InjectionToken<T>` which is a parameterized and type-safe
  version of `OpaqueToken`.

DEPRECATION:
- `OpaqueToken` is now deprecated, use `InjectionToken<T>` instead.
- `Injector.get(token: any, notFoundValue?: any): any` is now deprecated
  use the same method which is now overloaded as
  `Injector.get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T): T;`.

Migration
- Replace `OpaqueToken` with `InjectionToken<?>` and parameterize it.
- Migrate your code to only use `Type<?>` or `InjectionToken<?>` as
  injection tokens. Using other tokens will not be supported in the
  future.

BREAKING CHANGE:
- Because `injector.get()` is now parameterize it is possible that code
  which used to work no longer type checks. Example would be if one
  injects `Foo` but configures it as `{provide: Foo, useClass: MockFoo}`.
  The injection instance will be that of `MockFoo` but the type will be
  `Foo` instead of `any` as in the past. This means that it was possible
  to call a method on `MockFoo` in the past which now will fail type
  check. See this example:

```
class Foo {}
class MockFoo extends Foo {
  setupMock();
}

var PROVIDERS = [
  {provide: Foo, useClass: MockFoo}
];

...

function myTest(injector: Injector) {
  var foo = injector.get(Foo);
  // This line used to work since `foo` used to be `any` before this
  // change, it will now be `Foo`, and `Foo` does not have `setUpMock()`.
  // The fix is to downcast: `injector.get(Foo) as MockFoo`.
  foo.setUpMock();
}
```

@mhevery mhevery force-pushed the mhevery:injector-get branch from 66a6f96 to e8778e5 Jan 17, 2017

mhevery added a commit that referenced this pull request Jan 17, 2017

@mhevery mhevery closed this in d169c24 Jan 17, 2017

wKoza added a commit to wKoza/angular that referenced this pull request Feb 12, 2017

wKoza added a commit to wKoza/angular that referenced this pull request Feb 12, 2017

feat(core): Add type information to injector.get() (angular#13785)
- Introduce `InjectionToken<T>` which is a parameterized and type-safe
  version of `OpaqueToken`.

DEPRECATION:
- `OpaqueToken` is now deprecated, use `InjectionToken<T>` instead.
- `Injector.get(token: any, notFoundValue?: any): any` is now deprecated
  use the same method which is now overloaded as
  `Injector.get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T): T;`.

Migration
- Replace `OpaqueToken` with `InjectionToken<?>` and parameterize it.
- Migrate your code to only use `Type<?>` or `InjectionToken<?>` as
  injection tokens. Using other tokens will not be supported in the
  future.

BREAKING CHANGE:
- Because `injector.get()` is now parameterize it is possible that code
  which used to work no longer type checks. Example would be if one
  injects `Foo` but configures it as `{provide: Foo, useClass: MockFoo}`.
  The injection instance will be that of `MockFoo` but the type will be
  `Foo` instead of `any` as in the past. This means that it was possible
  to call a method on `MockFoo` in the past which now will fail type
  check. See this example:

```
class Foo {}
class MockFoo extends Foo {
  setupMock();
}

var PROVIDERS = [
  {provide: Foo, useClass: MockFoo}
];

...

function myTest(injector: Injector) {
  var foo = injector.get(Foo);
  // This line used to work since `foo` used to be `any` before this
  // change, it will now be `Foo`, and `Foo` does not have `setUpMock()`.
  // The fix is to downcast: `injector.get(Foo) as MockFoo`.
  foo.setUpMock();
}
```

PR Close angular#13785

@mhevery mhevery deleted the mhevery:injector-get branch Jun 2, 2017

juleskremer added a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

juleskremer added a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(core): Add type information to injector.get() (angular#13785)
- Introduce `InjectionToken<T>` which is a parameterized and type-safe
  version of `OpaqueToken`.

DEPRECATION:
- `OpaqueToken` is now deprecated, use `InjectionToken<T>` instead.
- `Injector.get(token: any, notFoundValue?: any): any` is now deprecated
  use the same method which is now overloaded as
  `Injector.get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T): T;`.

Migration
- Replace `OpaqueToken` with `InjectionToken<?>` and parameterize it.
- Migrate your code to only use `Type<?>` or `InjectionToken<?>` as
  injection tokens. Using other tokens will not be supported in the
  future.

BREAKING CHANGE:
- Because `injector.get()` is now parameterize it is possible that code
  which used to work no longer type checks. Example would be if one
  injects `Foo` but configures it as `{provide: Foo, useClass: MockFoo}`.
  The injection instance will be that of `MockFoo` but the type will be
  `Foo` instead of `any` as in the past. This means that it was possible
  to call a method on `MockFoo` in the past which now will fail type
  check. See this example:

```
class Foo {}
class MockFoo extends Foo {
  setupMock();
}

var PROVIDERS = [
  {provide: Foo, useClass: MockFoo}
];

...

function myTest(injector: Injector) {
  var foo = injector.get(Foo);
  // This line used to work since `foo` used to be `any` before this
  // change, it will now be `Foo`, and `Foo` does not have `setUpMock()`.
  // The fix is to downcast: `injector.get(Foo) as MockFoo`.
  foo.setUpMock();
}
```

PR Close angular#13785

@trotyl trotyl referenced this pull request Jul 31, 2018

Open

fix(core): allow abstract class as injection token #25222

2 of 3 tasks complete

Goodwine added a commit to Goodwine/angular that referenced this pull request Mar 13, 2019

fix(core): Deprecate TestBed.get(...):any
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - angular#13785

Issue angular#26491

@Goodwine Goodwine referenced this pull request Mar 13, 2019

Closed

fix(core): Deprecate TestBed.get(...):any #29290

6 of 14 tasks complete

Goodwine added a commit to Goodwine/angular that referenced this pull request Mar 13, 2019

fix(core): Deprecate TestBed.get(...):any
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - angular#13785

Issue angular#26491

Goodwine added a commit to Goodwine/angular that referenced this pull request Mar 13, 2019

fix(core): Deprecate TestBed.get(...):any
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - angular#13785

Issue angular#26491

Goodwine added a commit to Goodwine/angular that referenced this pull request Apr 3, 2019

fix(core): Deprecate TestBed.get(...):any
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - angular#13785

Issue angular#26491

IgorMinar added a commit that referenced this pull request Apr 4, 2019

fix(core): Deprecate TestBed.get(...):any (#29290)
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - #13785

Issue #26491

PR Close #29290

DeveloperFromUkraine added a commit to DeveloperFromUkraine/angular that referenced this pull request Apr 11, 2019

fix(core): Deprecate TestBed.get(...):any (angular#29290)
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - angular#13785

Issue angular#26491

PR Close angular#29290

wKoza added a commit to wKoza/angular that referenced this pull request Apr 17, 2019

fix(core): Deprecate TestBed.get(...):any (angular#29290)
Adds an overload to TestBed.get making parameters strongly typed and
deprecated previous signature that accepted types `any`. The function
still returns `any` to prevent build breakages, but eventually stronger
type checks will be added so a future Angular version will break builds
due to additional type checks.
See previous breaking change - angular#13785

Issue angular#26491

PR Close angular#29290
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.