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

Revert "fix(compiler): shouldn't throw when Symbol is used as DI toke… #15319

Closed
wants to merge 1 commit into from

Conversation

mhevery
Copy link
Contributor

@mhevery mhevery commented Mar 20, 2017

No description provided.

angular#13701)"

This reverts commit 8b5c6b2.

This feature is not compatible with the `Injector.get` which now only 
takes `Type` or `InjectableToken`. `Symbol` is not a valid type.

Closes angular#15183
@mhevery mhevery added action: merge The PR is ready for merge by the caretaker pr_action: queued labels Mar 20, 2017
@DzmitryShylovich
Copy link
Contributor

DzmitryShylovich commented Mar 20, 2017

@mhevery

This feature is not compatible with the Injector.get which now only
takes Type or InjectableToken.

what about strings? string can be used as a DI token but it's neither a Type nor an InjectionToken
http://plnkr.co/edit/fsg8igP78pl5JKABPhqQ?p=preview

Victor said anything can be used as a DI token. is it no longer true?

@tbosch
Copy link
Contributor

tbosch commented Mar 20, 2017

@DzmitryShylovich For JIT, anything can be used as DI token. But for AOT, only strings and OpaqueToken work...

@DzmitryShylovich
Copy link
Contributor

@tbosch

But for AOT, only strings and OpaqueToken work...

plus classes (Types) I guess.

ok, thanks.
shouldn't Injector.get signature be updated then?

get<T>(token: string|Type<T>|InjectionToken<T>, notFoundValue?: T): T

@DzmitryShylovich
Copy link
Contributor

@tbosch maybe it also makes sense to update Provider.provide type? Currently it's any.
because it's always confusing that something works in jit but doesn't work in aot and there's still no official docs what is supported and what is not. Plus ngc throws not very helpful error messages #15188 (comment)

@tbosch
Copy link
Contributor

tbosch commented Mar 21, 2017

@DzmitryShylovich Using string in get<T>(token: string|Type<T>|InjectionToken<T>, notFoundValue?: T): T does not enforce a type. So we will probably change the other get method, which current takes any to string...

We can't make provide in Provider anything else than any as this would break users.

@tbosch
Copy link
Contributor

tbosch commented Mar 21, 2017

@mhevery what do you think?

@mhevery
Copy link
Contributor Author

mhevery commented Mar 21, 2017

Hi @DzmitryShylovich Yes we are tightening the type and Injector.get and it will soon not take strings. It will only support Type and InjectionToken<Type>. We have to wait until v5 to drop the rest.

Yes you are right, we should tighten the typings on Provider as well. Do you want to take a stab at it?

@mhevery mhevery closed this in fa36ffd Mar 21, 2017
@DzmitryShylovich
Copy link
Contributor

@mhevery

Do you want to take a stab at it?

yeah. PR is here #15340

@mhevery mhevery deleted the revert branch June 2, 2017 17:08
asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017
angular#13701)" (angular#15319)

This reverts commit 8b5c6b2.

This feature is not compatible with the `Injector.get` which now only 
takes `Type` or `InjectableToken`. `Symbol` is not a valid type.

Closes angular#15183

PR Close angular#15319
juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017
angular#13701)" (angular#15319)

This reverts commit 8b5c6b2.

This feature is not compatible with the `Injector.get` which now only 
takes `Type` or `InjectableToken`. `Symbol` is not a valid type.

Closes angular#15183

PR Close angular#15319
@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
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 cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants