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

mhevery
Copy link
Contributor

@mhevery mhevery 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.

@mhevery mhevery force-pushed the injector-get branch 4 times, most recently from eea2e36 to 19089f5 Compare January 5, 2017 00:00
@pkozlowski-opensource pkozlowski-opensource added the area: core Issues related to the framework runtime label Jan 5, 2017
@mhevery mhevery requested review from vicb and IgorMinar January 5, 2017 17:44
@@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add docs for T

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

@vicb vicb Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vicb vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews pr_state: LGTM labels Jan 5, 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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

mhevery 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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not Token (or InjectionProvider, ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename opaqueToken ? (grep the whole project)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replaced

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for old OpaqueToken should be left as is.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@mhevery mhevery force-pushed the injector-get branch 2 times, most recently from 12f97ff to 66a6f96 Compare January 13, 2017 23:57
@mhevery mhevery removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 17, 2017
@mhevery mhevery modified the milestone: merg Jan 17, 2017
@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Jan 17, 2017
- 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 added a commit that referenced this pull request Jan 17, 2017
@mhevery mhevery closed this in d169c24 Jan 17, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
wKoza pushed a commit to wKoza/angular that referenced this pull request Feb 12, 2017
- 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 injector-get branch June 2, 2017 17:03
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
- 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
IgorMinar pushed a commit that referenced this pull request Apr 4, 2019
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 pushed a commit to DeveloperFromUkraine/angular that referenced this pull request Apr 11, 2019
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 pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
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
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
idlechara pushed a commit to idlechara/angular that referenced this pull request Apr 27, 2021
- 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants