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

feat(common): Add an 'outlet' injector option for ngTemplateOutlet #55389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mmalerba
Copy link
Contributor

@mmalerba mmalerba commented Apr 17, 2024

Adds an option (ngTemplateOutletInjector="outlet") that instructs the ngTemplateOutlet to inherit its injector from the outlet's place in the instantiated DOM.

@mmalerba mmalerba added the target: major This PR is targeted for the next major release label Apr 17, 2024
@mmalerba mmalerba requested a review from alxhub April 17, 2024 22:58
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Apr 17, 2024
@JeanMeche
Copy link
Member

Question: could this be a fix for #54097 ?

@mmalerba
Copy link
Contributor Author

Hmm I don't think so, the template outlet in that example looks like its already manually doing something pretty similar to what this would do. I tried adding an extra Injector.create to that example and it still had the same issue, so I think it would have the same issue with "inherit". This change was motivated by making it easier to build recursive structures like this: https://stackblitz.com/edit/stackblitz-starters-9csxbu?file=src%2Fmain.ts

Copy link
Contributor Author

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

@alxhub couple questions for you to make sure I'm on the right track

packages/common/src/directives/ng_template_outlet.ts Outdated Show resolved Hide resolved
private _getInjector(): Injector | undefined {
if (this.ngTemplateOutletInjector === 'inherit') {
this._childInjector =
this._childInjector ?? Injector.create({providers: [], parent: this.injector});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure why I need to create a child injector instead of just passing along the current injector, but based on my stackblitz testing it did seem to be necessary. Just wanted to call that out in case I'm doing something wrong

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be necessary I don't think...

Copy link
Contributor Author

@mmalerba mmalerba Apr 22, 2024

Choose a reason for hiding this comment

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

Here's a modified version of my stackblitz to show that it really does seem to need me to create a child to get the same behavior as composing with content projection: https://stackblitz.com/edit/stackblitz-starters-9csxbu?file=src%2Fmain.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked to @crisbeto about this. He says this behavior makes sense to him because embedded view injectors can't be injected using inject(Injector). By creating the child injector, we create an injector that acts as an embedded injector, but it ends up consulting the node injector for each lookup.

Adds an option (`ngTemplateOutletInjector="outlet"`) that instructs the
ngTemplateOutlet to inherit its injector from the outlet's place in the
instantiated DOM.
@mmalerba mmalerba added target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Apr 26, 2024
@mmalerba mmalerba changed the title feat(common): Add an inherit injector option for ngTemplateOutlet feat(common): Add an 'outlet' injector option for ngTemplateOutlet Apr 26, 2024
@thePunderWoman thePunderWoman added the area: common Issues related to APIs in the @angular/common package label Apr 30, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: common Issues related to APIs in the @angular/common package 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.

None yet

5 participants