-
Notifications
You must be signed in to change notification settings - Fork 27k
feat(core): introduce runInInjectionContext and deprecate prior version
#49396
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
cbce19a to
021be49
Compare
|
Wouldn't that solve #47566 ? |
packages/core/src/di/contextual.ts
Outdated
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.
OOC, is this ReturnT a Google stylistic suggestion? Never really saw this outside Google in any major project anyway. Traditionally this would be only T if it's just 1 type generic, or more specifically here TReturn, but never the other way around o:
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.
021be49 to
042b67a
Compare
atscott
left a comment
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.
reviewed-for: public-api, fw-core
jessicajaniuk
left a comment
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.
reviewed-for: fw-core, public-api
…sion With the introduction of `EnvironmentInjector`, we added an operation to run a function with access to `inject` tokens from that injector. This operation only worked for `EnvironmentInjector`s and not for element/node injectors. This commit deprecates `EnvironmentInjector.runInContext` in favor of a standalone API `runInInjectionContext`, which supports any type of injector. DEPRECATED: `EnvironmentInjector.runInContext` is now deprecated, with `runInInjectionContext` functioning as a direct replacement: ```typescript // Previous method version (deprecated): envInjector.runInContext(fn); // New standalone function: runInInjectionContext(envInjector, fn); ```
042b67a to
4548b4b
Compare
AndrewKushnir
left a comment
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.
👍
| } | ||
|
|
||
| private assertNotDestroyed(): void { | ||
| assertNotDestroyed(): 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.
| assertNotDestroyed(): void { | |
| /** @internal */ | |
| assertNotDestroyed(): 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.
R3Injector is already internal / not exported as public.
| // @deprecated (undocumented) | ||
| abstract get(token: any, notFoundValue?: any): any; | ||
| // @deprecated | ||
| abstract runInContext<ReturnT>(fn: () => ReturnT): ReturnT; |
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 it'd be great to reuse the runInInjectionContext function within the EnvironmentInjector.runInContext implementation (to avoid code duplication):
angular/packages/core/src/di/r3_injector.ts
Lines 217 to 228 in 7dbe328
| override runInContext<ReturnT>(fn: () => ReturnT): ReturnT { | |
| this.assertNotDestroyed(); | |
| const previousInjector = setCurrentInjector(this); | |
| const previousInjectImplementation = setInjectImplementation(undefined); | |
| try { | |
| return fn(); | |
| } finally { | |
| setCurrentInjector(previousInjector); | |
| setInjectImplementation(previousInjectImplementation); | |
| } | |
| } |
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 isn't possible because runInInjectionContext depends on R3Injector so that would be a circular reference.
|
This PR was merged into the repository by commit 0814f20. |
|
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. |
With the introduction of
EnvironmentInjector, we added an operation to run a function with access toinjecttokens from that injector. This operation only worked forEnvironmentInjectors and not for element/node injectors.This commit deprecates
EnvironmentInjector.runInContextin favor of a standalone APIrunInInjectionContext, which supports any type of injector.DEPRECATION:
EnvironmentInjector.runInContextis now deprecated, withrunInInjectionContextfunctioning as a direct replacement: