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): Refactor out constants to be used in jsaction. Also, don't do anymore jsaction work after event replay is done. #55799

Closed
wants to merge 8 commits into from
Closed

refactor(core): Refactor out constants to be used in jsaction. Also, don't do anymore jsaction work after event replay is done. #55799

wants to merge 8 commits into from

Conversation

iteriani
Copy link
Contributor

@iteriani iteriani commented May 14, 2024 •
edited

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 atscott May 14, 2024 20:24
@iteriani iteriani requested review from AndrewKushnir and removed request for atscott May 14, 2024 20:34
@pullapprove pullapprove bot requested a review from csmick May 14, 2024 22:18
@pullapprove pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label May 14, 2024
@iteriani iteriani force-pushed the tests2 branch 2 times, most recently from 38624da to f725ee4 Compare May 14, 2024 22:27
@iteriani iteriani requested review from tbondwilkinson and removed request for csmick May 14, 2024 22:27
@iteriani iteriani changed the title refactor(core): Add a test case for content projection. refactor(core): Refactor out constants to be used in jsaction. Also, don't do anymore jsaction work after event replay is done. May 14, 2024
AndrewKushnir
AndrewKushnir reviewed May 15, 2024
View reviewed changes
goldens/public-api/core/primitives/event-dispatch/index.md Outdated Show resolved Hide resolved
packages/core/src/hydration/annotate.ts Outdated Show resolved Hide resolved
packages/core/src/hydration/annotate.ts Outdated Show resolved Hide resolved
packages/core/src/hydration/event_replay.ts Show resolved Hide resolved
packages/core/src/hydration/event_replay.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir mentioned this pull request May 15, 2024
Closed
@iteriani
refactor(core): Make sure not to do anymore jsaction work once everyt…
702992b 
…hing is replayed.

Without this, I think subsequent renders will populate data structures.
@iteriani
refactor(core): Create event types that are able to be serialized, ca…
7496f88 
…ptured, or are mouse events.

Use these constants across jsaction and Angular.
@iteriani
fixup! refactor(core): Create event types that are able to be seriali…
b3201de 
…zed, captured, or are mouse events.
@iteriani
fixup! refactor(core): Create event types that are able to be seriali…
835cf05 
…zed, captured, or are mouse events.
AndrewKushnir
AndrewKushnir approved these changes May 17, 2024
View reviewed changes
packages/core/primitives/event-dispatch/src/event_type.ts Outdated
]);

/**
* A default set of events that clients should use to install the event contract.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* A default set of events that clients should use to install the event contract.
* Detects whether a given event type is supported by JSAction.

AndrewKushnir
AndrewKushnir approved these changes May 17, 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

thePunderWoman
thePunderWoman approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

reviewed-for: fw-core, fw-platform-server, primitives, public-api

@iteriani
Copy link
Contributor Author

TESTED=TGP

AndrewKushnir
AndrewKushnir approved these changes May 22, 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.

@iteriani LGTM 👍

There was a tiny comment related to docs: https://github.com/angular/angular/pull/55799/files#r1603875571, but we can address it in a separate PR if it'd be easier.

@AndrewKushnir AndrewKushnir added target: patch This PR is targeted for the next patch release area: core Issues related to the framework runtime core: event dispatch labels May 22, 2024
@ngbot ngbot bot modified the milestone: Backlog May 22, 2024
@iteriani iteriani added the action: merge The PR is ready for merge by the caretaker label May 22, 2024
@pkozlowski-opensource pkozlowski-opensource added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 23, 2024
@pkozlowski-opensource
Copy link
Member

caretaker note: pending code reviews status got "stuck", ignoring pending reviews

@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 690b0fb.

pkozlowski-opensource pushed a commit that referenced this pull request May 23, 2024
@iteriani @pkozlowski-opensource
refactor(core): Make sure not to do anymore jsaction work once everyt…
01a19f3 
…hing is replayed. (#55799)

Without this, I think subsequent renders will populate data structures.

PR Close #55799
pkozlowski-opensource pushed a commit that referenced this pull request May 23, 2024
@iteriani @pkozlowski-opensource
refactor(core): Create event types that are able to be serialized, ca…
89e48aa 
…ptured, or are mouse events. (#55799)

Use these constants across jsaction and Angular.

PR Close #55799
pkozlowski-opensource pushed a commit that referenced this pull request May 23, 2024
@iteriani @pkozlowski-opensource
refactor(core): Create event types that are able to be serialized, ca…
690b0fb 
…ptured, or are mouse events. (#55799)

Use these constants across jsaction and Angular.

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

@tbondwilkinson tbondwilkinson tbondwilkinson approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

@dylhunn dylhunn Awaiting requested review from dylhunn

@thePunderWoman thePunderWoman Awaiting requested review from thePunderWoman

Assignees

@tbondwilkinson tbondwilkinson

@AndrewKushnir AndrewKushnir

Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime core: event dispatch merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note requires: TGP This PR requires a passing TGP before merging is allowed target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@iteriani @pkozlowski-opensource @thePunderWoman @tbondwilkinson @AndrewKushnir