Skip to content

feat(core): introduce runInInjectionContext and deprecate prior version#49396

Closed
alxhub wants to merge 1 commit intoangular:mainfrom
alxhub:fw/reactivity/runininjectioncontext
Closed

feat(core): introduce runInInjectionContext and deprecate prior version#49396
alxhub wants to merge 1 commit intoangular:mainfrom
alxhub:fw/reactivity/runininjectioncontext

Conversation

@alxhub
Copy link
Copy Markdown
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
Copy Markdown
Member

JeanMeche commented Mar 10, 2023

Wouldn't that solve #47566 ?

alfaproject
alfaproject reviewed Mar 12, 2023
View reviewed changes
packages/core/src/di/contextual.ts Outdated
Copy link
Copy Markdown
Contributor

@alfaproject alfaproject Mar 12, 2023

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
Copy Markdown
Member

@JeanMeche JeanMeche Mar 12, 2023

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
atscott
atscott approved these changes Mar 13, 2023
View reviewed changes
Copy link
Copy Markdown
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

jessicajaniuk
jessicajaniuk approved these changes Mar 13, 2023
View reviewed changes
Copy link
Copy Markdown
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

@alxhub
feat(core): introduce runInInjectionContext and deprecate prior ver…
4548b4b 
…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
AndrewKushnir
AndrewKushnir approved these changes Mar 13, 2023
View reviewed changes
Copy link
Copy Markdown
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.

👍

packages/core/src/di/r3_injector.ts
}

private assertNotDestroyed(): void {
assertNotDestroyed(): void {
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir Mar 13, 2023

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
Copy Markdown
Member Author

@alxhub alxhub Mar 13, 2023 •
edited
Loading

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.

goldens/public-api/core/index.md
// @deprecated (undocumented)
abstract get(token: any, notFoundValue?: any): any;
// @deprecated
abstract runInContext<ReturnT>(fn: () => ReturnT): ReturnT;
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir Mar 13, 2023

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);
}
}

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

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
Copy Markdown
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
Copy Markdown

angular-automatic-lock-bot bot commented Apr 14, 2023

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.

Reviewers

@JeanMeche JeanMeche JeanMeche left review comments

@atscott atscott atscott approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@pkozlowski-opensource pkozlowski-opensource Awaiting requested review from pkozlowski-opensource

+2 more reviewers

@alfaproject alfaproject alfaproject left review comments

@jessicajaniuk jessicajaniuk jessicajaniuk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@atscott atscott

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@alxhub @JeanMeche @alfaproject @atscott @AndrewKushnir @jessicajaniuk