Skip to content

Conversation

@alxhub
Copy link
Member

@alxhub alxhub commented Mar 10, 2023

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 EnvironmentInjectors and not for element/node injectors.

This commit deprecates EnvironmentInjector.runInContext in favor of a standalone API runInInjectionContext, which supports any type of injector.

DEPRECATION:

EnvironmentInjector.runInContext is now deprecated, with runInInjectionContext functioning as a direct replacement:

// Previous method version (deprecated):
envInjector.runInContext(fn);
// New standalone function:
runInInjectionContext(envInjector, fn);

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Mar 10, 2023
@alxhub alxhub force-pushed the fw/reactivity/runininjectioncontext branch from cbce19a to 021be49 Compare March 10, 2023 19:23
@JeanMeche
Copy link
Member

Wouldn't that solve #47566 ?

Copy link
Contributor

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:

Copy link
Member

Choose a reason for hiding this comment

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

fwiw: 400ish vs 12K occurences on GH

@alxhub alxhub force-pushed the fw/reactivity/runininjectioncontext branch from 021be49 to 042b67a Compare March 13, 2023 16:42
@angular-robot angular-robot bot added the detected: deprecation PR contains a commit with a deprecation label Mar 13, 2023
@alxhub alxhub marked this pull request as ready for review March 13, 2023 16:46
Copy link
Contributor

@atscott atscott left a 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

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a 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);
```
@alxhub alxhub force-pushed the fw/reactivity/runininjectioncontext branch from 042b67a to 4548b4b Compare March 13, 2023 17:26
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assertNotDestroyed(): void {
/** @internal */
assertNotDestroyed(): void {

Copy link
Member Author

@alxhub alxhub Mar 13, 2023

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

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):

override runInContext<ReturnT>(fn: () => ReturnT): ReturnT {
this.assertNotDestroyed();
const previousInjector = setCurrentInjector(this);
const previousInjectImplementation = setInjectImplementation(undefined);
try {
return fn();
} finally {
setCurrentInjector(previousInjector);
setInjectImplementation(previousInjectImplementation);
}
}

Copy link
Member Author

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.

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Mar 13, 2023
@alxhub
Copy link
Member Author

alxhub commented Mar 14, 2023

This PR was merged into the repository by commit 0814f20.

@alxhub alxhub closed this in 0814f20 Mar 14, 2023
@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 Apr 14, 2023
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 detected: deprecation PR contains a commit with a deprecation detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants