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.

Sign up for GitHub

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

Jump to bottom

refactor(core): Remove jsaction from element after handling the event. #55549

Closed
wants to merge 2 commits into from
Closed

refactor(core): Remove jsaction from element after handling the event. #55549

wants to merge 2 commits into from

Conversation

iteriani
Copy link
Contributor

This also adds a test to make sure that the event contract is still listening to other events, especially in the case where we may want partial hydration in the future.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested a review from dylhunn April 25, 2024 21:15
@iteriani iteriani force-pushed the yeah branch 3 times, most recently from 8840c70 to b2193f3 Compare April 25, 2024 23:16
@AndrewKushnir AndrewKushnir removed the request for review from dylhunn April 25, 2024 23:19
AndrewKushnir
AndrewKushnir reviewed Apr 26, 2024
View reviewed changes
Copy link
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.

Looks great, just left a few comments/proposals.

packages/core/src/hydration/event_replay.ts Outdated Show resolved Hide resolved
packages/core/src/hydration/event_replay.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/listener.ts Outdated Show resolved Hide resolved
packages/core/src/render3/instructions/listener.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: rc This PR is targeted for the next release-candidate core: event dispatch labels Apr 26, 2024
@ngbot ngbot bot added this to the Backlog milestone Apr 26, 2024
@pullapprove pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label Apr 26, 2024
@iteriani iteriani 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 labels Apr 26, 2024
@iteriani
Copy link
Contributor Author

caretaker note: this pr is only adding to index.ts, so this shouldn't affect g3.

@pullapprove pullapprove bot requested a review from alxhub April 26, 2024 22:21
AndrewKushnir
AndrewKushnir approved these changes Apr 26, 2024
View reviewed changes
Copy link
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.

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label Apr 26, 2024
@AndrewKushnir
Copy link
Contributor

FYI, removing the "merge" label for now (to exclude the PR from the merge queue), since we'd need an LGTM from the primitives-shared group and one more public-api review.

thePunderWoman
thePunderWoman approved these changes Apr 29, 2024
View reviewed changes
Copy link
Contributor

@thePunderWoman thePunderWoman 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

@iteriani
refactor(core): Remove jsaction from element after handling the event.
ebdb449 
This also adds a test to make sure that the event contract is still listening to other events, especially in the case where we may want partial hydration in the future.
AndrewKushnir
AndrewKushnir approved these changes Apr 30, 2024
View reviewed changes
goldens/public-api/core/primitives/event-dispatch/index.md Outdated
@@ -4,6 +4,17 @@

```ts

// @public (undocumented)
export enum Attribute {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can consider making separate consts for each string to improve tree-shaking.

AndrewKushnir
AndrewKushnir approved these changes Apr 30, 2024
View reviewed changes
Copy link
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.

Reviewed-for: public-api

@pullapprove pullapprove bot removed the requires: TGP This PR requires a passing TGP before merging is allowed label May 1, 2024
@iteriani iteriani 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 May 2, 2024
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit 95bf0c8.

AndrewKushnir pushed a commit that referenced this pull request May 2, 2024
@iteriani @AndrewKushnir
refactor(core): Remove jsaction from element after handling the event. (
8432d75 
#55549)

This also adds a test to make sure that the event contract is still listening to other events, especially in the case where we may want partial hydration in the future.

PR Close #55549
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thePunderWoman thePunderWoman thePunderWoman approved these changes

@tbondwilkinson tbondwilkinson tbondwilkinson approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir 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 core: event dispatch 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
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@iteriani @AndrewKushnir @thePunderWoman @tbondwilkinson