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): handle hydration of view containers for root components #51247

Closed
wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Aug 2, 2023

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the | char (e.g. 10|25, where 10 is the ngh for a component view and 25 is the ngh for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves #51157.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@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: hydration labels Aug 2, 2023
@ngbot ngbot bot added this to the Backlog milestone Aug 2, 2023
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Aug 2, 2023
@AndrewKushnir AndrewKushnir marked this pull request as ready for review August 2, 2023 05:37
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.

LGTM!

@jessicajaniuk jessicajaniuk 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 Aug 2, 2023
@ngbot
Copy link

ngbot bot commented Aug 2, 2023

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "ci/circleci: test" is failing
    failure status "google-internal-tests" is failing
    pending status "mergeability" is pending
    pending status "pullapprove" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Aug 2, 2023
@AndrewKushnir
Copy link
Contributor Author

Presubmit.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 2, 2023
@AndrewKushnir
Copy link
Contributor Author

Caretaker note: presubmit is "green", this PR is ready for merge.

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.
@AndrewKushnir AndrewKushnir added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Aug 4, 2023
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Aug 4, 2023

Caretaker note 2: please merge PR #51276 first (before merging this PR), it would allow this PR to merge cleanly into 16.2.x branch.

// cc @dylhunn

@dylhunn
Copy link
Contributor

dylhunn commented Aug 7, 2023

This PR was merged into the repository by commit 55965cb.

dylhunn pushed a commit that referenced this pull request Aug 7, 2023
…51247)

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves #51157.

PR Close #51247
@dylhunn dylhunn closed this in 55965cb Aug 7, 2023
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Aug 22, 2023
…ts as anchors

This commit fixes an issue where serialization of a view container fails in case it uses a component host as an anchor. This fix is similar to the fix from angular#51247, but for cases when we insert a component (that acts as a host for a view container) deeper in a hierarchy.

Resolves angular#51318.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Aug 22, 2023
…ts as anchors

This commit fixes an issue where serialization of a view container fails in case it uses a component host as an anchor. This fix is similar to the fix from angular#51247, but for cases when we insert a component (that acts as a host for a view container) deeper in a hierarchy.

Resolves angular#51318.
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Aug 28, 2023
…ts as anchors

This commit fixes an issue where serialization of a view container fails in case it uses a component host as an anchor. This fix is similar to the fix from angular#51247, but for cases when we insert a component (that acts as a host for a view container) deeper in a hierarchy.

Resolves angular#51318.
thomasturrell pushed a commit to thomasturrell/angular that referenced this pull request Aug 29, 2023
…ngular#51247)

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.

PR Close angular#51247
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…ts as anchors (#51456)

This commit fixes an issue where serialization of a view container fails in case it uses a component host as an anchor. This fix is similar to the fix from #51247, but for cases when we insert a component (that acts as a host for a view container) deeper in a hierarchy.

Resolves #51318.

PR Close #51456
jessicajaniuk pushed a commit that referenced this pull request Aug 29, 2023
…ts as anchors (#51456)

This commit fixes an issue where serialization of a view container fails in case it uses a component host as an anchor. This fix is similar to the fix from #51247, but for cases when we insert a component (that acts as a host for a view container) deeper in a hierarchy.

Resolves #51318.

PR Close #51456
@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 Sep 7, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
…ngular#51247)

For cases when a root component also acts as an anchor node for a ViewContainerRef (for example, when ViewContainerRef is injected in a root component), there is a need to serialize information about the component itself, as well as an LContainer that represents this ViewContainerRef. Effectively, we need to serialize 2 pieces of info: (1) hydration info for the root component itself and (2) hydration info for the ViewContainerRef instance (an LContainer). Each piece of information is included into the hydration data (in the TransferState object) separately, thus we end up with 2 ids. Since we only have 1 root element, we encode both bits of info into a single string: ids are separated by the `|` char (e.g. `10|25`, where `10` is the ngh for a component view and 25 is the `ngh` for a root view which holds LContainer).

Previously, we were only including component-related information, thus all the views in the view container remained dehydrated and duplicated (client-rendered from scratch) on the client.

Resolves angular#51157.

PR Close angular#51247
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 area: core Issues related to the framework runtime core: hydration merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic components aren't hydrated when root component is used as an anchor
3 participants