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

fix(core): establish proper injector resolution order for @defer blocks #55079

Closed

Conversation

AndrewKushnir
Copy link
Contributor

This commit updates the @defer logic to establish proper injector resolution order. More specifically:

  • Makes node injectors inspected first, similar to how it happens when @defer block is not used.
  • Adds extra handling for the Router's OutletInjector, until we replace it with an EnvironmentInjector.

Resolves #54864.
Resolves #55028.
Resolves #55036.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

…ocks

This commit updates the `@defer` logic to establish proper injector resolution order. More specifically:

- Makes node injectors to be inspected first, similar to how it happens when `@defer` block is not used.
- Adds extra handling for the Router's `OutletInjector`, until we replace it with an `EnvironmentInjector`.

Resolves angular#54864.
Resolves angular#55028.
Resolves angular#55036.
@AndrewKushnir AndrewKushnir added state: WIP area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release core: defer Issues related to @defer blocks. labels Mar 28, 2024
@ngbot ngbot bot added this to the Backlog milestone Mar 28, 2024
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Mar 28, 2024
@AndrewKushnir AndrewKushnir marked this pull request as ready for review March 28, 2024 01:04
@AndrewKushnir
Copy link
Contributor Author

Exploratory presubmit.

@AndrewKushnir AndrewKushnir removed the request for review from atscott March 28, 2024 16:09
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 28, 2024
@dylhunn
Copy link
Contributor

dylhunn commented Mar 28, 2024

This PR was merged into the repository by commit 86a359b.

@dylhunn dylhunn closed this in 86a359b Mar 28, 2024
dylhunn pushed a commit that referenced this pull request Mar 28, 2024
…ocks (#55079)

This commit updates the `@defer` logic to establish proper injector resolution order. More specifically:

- Makes node injectors to be inspected first, similar to how it happens when `@defer` block is not used.
- Adds extra handling for the Router's `OutletInjector`, until we replace it with an `EnvironmentInjector`.

Resolves #54864.
Resolves #55028.
Resolves #55036.

PR Close #55079
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Apr 1, 2024
The fix from PR angular#55079 introduced a configuration of the injector chain, which wasn't properly handled by the injector debug utils, thus resulting in JS exceptions in DevTools. This commit updates injector debug utils logic that calculates injector resolution path to also handle `ChainedInjector`s.

Resolves angular#55137.
jessicajaniuk pushed a commit that referenced this pull request Apr 3, 2024
The fix from PR #55079 introduced a configuration of the injector chain, which wasn't properly handled by the injector debug utils, thus resulting in JS exceptions in DevTools. This commit updates injector debug utils logic that calculates injector resolution path to also handle `ChainedInjector`s.

Resolves #55137.

PR Close #55144
jessicajaniuk pushed a commit that referenced this pull request Apr 3, 2024
The fix from PR #55079 introduced a configuration of the injector chain, which wasn't properly handled by the injector debug utils, thus resulting in JS exceptions in DevTools. This commit updates injector debug utils logic that calculates injector resolution path to also handle `ChainedInjector`s.

Resolves #55137.

PR Close #55144
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
…ocks (angular#55079)

This commit updates the `@defer` logic to establish proper injector resolution order. More specifically:

- Makes node injectors to be inspected first, similar to how it happens when `@defer` block is not used.
- Adds extra handling for the Router's `OutletInjector`, until we replace it with an `EnvironmentInjector`.

Resolves angular#54864.
Resolves angular#55028.
Resolves angular#55036.

PR Close angular#55079
ilirbeqirii pushed a commit to ilirbeqirii/angular that referenced this pull request Apr 6, 2024
…#55144)

The fix from PR angular#55079 introduced a configuration of the injector chain, which wasn't properly handled by the injector debug utils, thus resulting in JS exceptions in DevTools. This commit updates injector debug utils logic that calculates injector resolution path to also handle `ChainedInjector`s.

Resolves angular#55137.

PR Close angular#55144
@simeyla
Copy link

simeyla commented Apr 17, 2024

@AndrewKushnir I believe this fix may have broken the resolution of services provided above the level of @defer when @defer appears inside a dynamic component (in my case specifically ComponentPortal from cdk.

I know there have been some issues before with dynamic components being forgotten when refactoring injector resolution - and I suspect the same happened here. I'm happy to create a simple repro if what I'm saying doesn't resonate.

Basically the repro would look like:

AppComponent provides TestService
MainComponent is dynamically created with ComponentPortal and added to AppComponent
MainComponent contains @defer block containing an instance of ChildComponent
ChildComponent injects TestService

Expected ERROR:

ChildComponent cannot resolve TestService with No provider for _TestService!.
Removing the @defer fixes it.

There may or may not additional complexity in my app but I suspect this to be the basic structure needed.

@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Apr 17, 2024

@simeyla thanks for reporting the issue. It'd be helpful if you could create a new ticket and provide a minimal repro (in a form of GitHub repository) with a few components (ideally using ViewContainerRef APIs to create components dynamically, i.e. without using CDK), so that we can perform further investigation.

I believe this fix may have broken the resolution of services provided above the level of @defer when @defer appears inside a dynamic component (in my case specifically ComponentPortal from cdk.

Could you please confirm if the issue was not present before the fix (in earlier version)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime core: defer Issues related to @defer blocks. target: patch This PR is targeted for the next patch release
Projects
None yet
5 participants