-
Notifications
You must be signed in to change notification settings - Fork 26.2k
fix(di): add better type information to injector.get() #13785
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
Conversation
eea2e36
to
19089f5
Compare
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ?
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add docs for T
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed ?
There was a problem hiding this comment.
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.
@@ -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'); |
There was a problem hiding this comment.
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>>
.
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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>>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this 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?
4d95a76
to
dce3247
Compare
@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'; |
There was a problem hiding this comment.
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
, ...)
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
There was a problem hiding this comment.
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.
There was a problem hiding this 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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
12f97ff
to
66a6f96
Compare
- 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(); } ```
- 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
- 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
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
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
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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
- 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
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 ofFoo
injector.get(new OpaqueToken<Foo>(‘Foo’))
returns an instance ofFoo
.Migration
OpaqueToken
, it must now be parameterized.type other than itself. For example
var cars: Car[] = injector.get(Car)
because the provider forCar
was declared
multi: true
. This is now no longer possible and it isconsidered a bad form. Proper way to do this is through
var CARS = new OpaqueToken<Car[]>;
and thenvar cars = injector.get(CARS)
.var foo: any = injector.get(‘something’)
is still supported andresults in
any
type, but it’s use is strongly discouraged.