Skip to content

fix(core): clean up event contract once hydration is done - #58174

Closed
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/event_contract_cleanup
Closed

fix(core): clean up event contract once hydration is done#58174
arturovt wants to merge 1 commit into
angular:mainfrom
arturovt:fix/event_contract_cleanup

Conversation

@arturovt

@arturovt arturovt commented Oct 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

In this commit, we clean up the event contract once hydration is complete, which removes event
listeners registered through the container manager. If we do not clean up the contract, the listeners
will remain on the document.body. When incremental hydration is enabled, we cannot clean up the event
contract immediately; instead, we schedule its cleanup when the app is destroyed. This is because the
event contract is required for deferred blocks, of which we are unaware, that need to be hydrated.

@angular-robot angular-robot Bot added the area: core Issues related to the framework runtime label Oct 11, 2024
@ngbot ngbot Bot added this to the Backlog milestone Oct 11, 2024
AndrewKushnir
AndrewKushnir reviewed Oct 12, 2024
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arturovt thanks for the PR 👍 I think there might be some overlap with the code of the Incremental Hydration feature (see #56311) and this change would probably require some modifications after that. We'll keep the PR open and re-review it once the Incremental Hydration code lands.

Comment thread packages/core/src/hydration/event_replay.ts Outdated
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from 18a0b83 to d60da23 Compare October 22, 2024 15:41
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 22, 2024
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from d60da23 to 88caa78 Compare October 22, 2024 20:30
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 22, 2024
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from 88caa78 to 2c9959d Compare October 23, 2024 17:16
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 23, 2024
@arturovt
arturovt marked this pull request as ready for review October 23, 2024 17:16
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from 2c9959d to eec9e4a Compare October 27, 2024 02:46
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 27, 2024
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from eec9e4a to b141989 Compare October 27, 2024 02:48
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 27, 2024
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from b141989 to f852f6b Compare October 29, 2024 00:06
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 29, 2024
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from f852f6b to 1bdfd1d Compare November 4, 2024 00:34
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Nov 4, 2024
@thePunderWoman

thePunderWoman commented Nov 11, 2024

Copy link
Copy Markdown
Contributor

@arturovt I think we should be safe to land this now. Can you rebase and resolve the conflict?

@arturovt
fix(core): clean up event contract once hydration is done
32bc96f 
In this commit, we clean up the event contract once hydration is complete, which removes event
listeners registered through the container manager. If we do not clean up the contract, the listeners
will remain on the `document.body`. When incremental hydration is enabled, we cannot clean up the event
contract immediately; instead, we schedule its cleanup when the app is destroyed. This is because the
event contract is required for deferred blocks, of which we are unaware, that need to be hydrated.
@arturovt
arturovt force-pushed the fix/event_contract_cleanup branch from 1bdfd1d to 32bc96f Compare November 11, 2024 22:01
@angular-robot angular-robot Bot removed the area: core Issues related to the framework runtime label Nov 11, 2024
@angular-robot angular-robot Bot added the area: core Issues related to the framework runtime label Nov 11, 2024
thePunderWoman
thePunderWoman approved these changes Nov 11, 2024
View reviewed changes

@thePunderWoman thePunderWoman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks!

@thePunderWoman thePunderWoman added target: rc This PR is targeted for the next release-candidate action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit action: merge The PR is ready for merge by the caretaker labels Nov 11, 2024
thePunderWoman pushed a commit that referenced this pull request Nov 12, 2024
@arturovt @thePunderWoman
fix(core): clean up event contract once hydration is done (#58174)
71ee81a 
In this commit, we clean up the event contract once hydration is complete, which removes event
listeners registered through the container manager. If we do not clean up the contract, the listeners
will remain on the `document.body`. When incremental hydration is enabled, we cannot clean up the event
contract immediately; instead, we schedule its cleanup when the app is destroyed. This is because the
event contract is required for deferred blocks, of which we are unaware, that need to be hydrated.

PR Close #58174
@thePunderWoman

thePunderWoman commented Nov 12, 2024

Copy link
Copy Markdown
Contributor

This PR was merged into the repository by commit 4df352b.

The changes were merged into the following branches: main, 19.0.x

@arturovt
arturovt deleted the fix/event_contract_cleanup branch November 12, 2024 15:16
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Dec 13, 2024

Copy link
Copy Markdown

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 Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir left review comments
@thePunderWoman thePunderWoman thePunderWoman approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

3 participants

@arturovt @thePunderWoman @AndrewKushnir