refactor(core): Use the early event contract instead of the event contract in the bootstrap. #55587

iteriani · 2024-04-30T00:23:32Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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

AndrewKushnir

@iteriani I think there is some logic missing for the serialization phase: we'd need to exclude all mouse-related events (as that happens today in the EventContract) and avoid including them into the contract even if those events were defined in a template. We may also need to do some additional processing (normalizing event names, etc) that happens today in the EventContract.

packages/core/primitives/event-dispatch/src/register_events.ts

tbondwilkinson

You may also want a test that shows that capture events do not work for EarlyEventContract.

packages/core/primitives/event-dispatch/src/earlyeventcontract.ts

packages/core/src/hydration/event_replay.ts

packages/platform-server/test/event_replay_spec.ts

all comments resolved; capture tests have been added

iteriani · 2024-05-06T20:31:03Z

Should be available for review again

packages/core/primitives/event-dispatch/src/register_events.ts

packages/core/primitives/event-dispatch/terser.config.json

packages/core/src/hydration/event_replay.ts

AndrewKushnir · 2024-05-07T20:43:35Z

@iteriani could you please rebase this PR on top of the most recent main branch? We've merged PR #55708 (comment), which adds size-tracking to the event contract script and it'd likely decrease a lot (which is great!) with the changes from this PR. Once you rebase and push the changes, we'll see what CI says and we can update the size-trackingaccordingly.

…event types before adding them. In some cases, we will be passing in undefined for capture events, so handle this.

…t in bootstrap. This also fixes an existing bug where we erase the jsaction attribute too early. Now the event contract binary is 608 bytes :D.

…contract in bootstrap.

AndrewKushnir

@iteriani thanks for addressing the feedback!

I've left a comment, but it's not blocking, we can do it in a followup PR (it may also require some changes from the JSAction side).

AndrewKushnir · 2024-05-07T23:00:31Z

packages/platform-server/src/utils.ts

+    for (const eventType of events) {
+      if (
+        eventType === 'mouseenter' ||
+        eventType === 'mouseleave' ||
+        eventType === 'pointerenter' ||
+        eventType === 'pointerleave'
+      ) {
+        continue;
+      }
+      if (
+        eventType === 'focus' ||
+        eventType === 'blur' ||
+        eventType === 'error' ||
+        eventType === 'load' ||
+        eventType === 'toggle'
+      ) {
+        captureEventTypes.push(eventType);


nit: just as an idea: we can have a couple helper functions from JSAction code, so that we can keep those lists in sync with how JSAction uses them internally.

Suggested change

for (const eventType of events) {

if (

eventType === 'mouseenter' ||

eventType === 'mouseleave' ||

eventType === 'pointerenter' ||

eventType === 'pointerleave'

) {

continue;

}

if (

eventType === 'focus' ||

eventType === 'blur' ||

eventType === 'error' ||

eventType === 'load' ||

eventType === 'toggle'

) {

captureEventTypes.push(eventType);

for (const eventType of events) {

if (isMouseEvent(eventType) {

continue;

}

if (useCapturePhase(eventType)) {

captureEventTypes.push(eventType);

AndrewKushnir

Reviewed-for: public-api

thePunderWoman

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

iteriani · 2024-05-08T16:15:36Z

TGP

thePunderWoman · 2024-05-08T16:29:41Z

goldens/size-tracking/integration-payloads.json

@@ -54,9 +54,9 @@
  },
  "platform-server-hydration/browser": {
    "uncompressed": {
-      "main": 200584,
+      "main": 207890,


I'm confused why main jumped so much. I would have thought this only would impact the event-dispatch size. Any idea what's up there?

we moved logic that was in the event contract to the main bundle.

thePunderWoman

reviewed-for: size-tracking

iteriani · 2024-05-08T19:42:35Z

TESTED=TGP

atscott · 2024-05-09T21:34:13Z

This PR was merged into the repository by commit 28fb385.

…event types before adding them. (#55587) In some cases, we will be passing in undefined for capture events, so handle this. PR Close #55587

…t in bootstrap. (#55587) This also fixes an existing bug where we erase the jsaction attribute too early. Now the event contract binary is 608 bytes :D. PR Close #55587

…t in bootstrap. (angular#55587) This also fixes an existing bug where we erase the jsaction attribute too early. Now the event contract binary is 608 bytes :D. PR Close angular#55587

angular-automatic-lock-bot · 2024-06-10T00:09:51Z

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.}

iteriani requested review from tbondwilkinson and AndrewKushnir April 30, 2024 00:23

pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label Apr 30, 2024

AndrewKushnir reviewed Apr 30, 2024

View reviewed changes

packages/core/primitives/event-dispatch/src/register_events.ts Outdated Show resolved Hide resolved

thePunderWoman 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 labels Apr 30, 2024

ngbot bot modified the milestone: Backlog Apr 30, 2024

tbondwilkinson previously requested changes Apr 30, 2024

View reviewed changes

iteriani force-pushed the eventcontractbinary2 branch 2 times, most recently from 43dc492 to dc373b7 Compare April 30, 2024 22:24

iteriani force-pushed the eventcontractbinary2 branch 5 times, most recently from 5cac9cc to 83f10ef Compare May 6, 2024 20:21

iteriani mentioned this pull request May 6, 2024

refactor(core): Export some more symbols and check for truthiness on … #55691

Closed

14 tasks

tbondwilkinson approved these changes May 6, 2024

View reviewed changes

AndrewKushnir reviewed May 6, 2024

View reviewed changes

alan-agius4 mentioned this pull request May 7, 2024

build: switch from rollup and terser to esbuild for creating contract bundle #55705

Closed

iteriani force-pushed the eventcontractbinary2 branch from 1066be4 to a6aae74 Compare May 7, 2024 15:22

AndrewKushnir mentioned this pull request May 7, 2024

test: add Integration tests for hydration and event reply #55708

Closed

iteriani added 4 commits May 7, 2024 15:02

refactor(core): Export some more symbols and check for truthiness on …

13cb6e5

…event types before adding them. In some cases, we will be passing in undefined for capture events, so handle this.

refactor(core): Use early event contract instead of the event contrac…

c6914bf

…t in bootstrap. This also fixes an existing bug where we erase the jsaction attribute too early. Now the event contract binary is 608 bytes :D.

fixup! refactor(core): Use early event contract instead of the event …

4fb0361

…contract in bootstrap.

fixup! refactor(core): Use early event contract instead of the event …

4cace25

…contract in bootstrap.

AndrewKushnir approved these changes May 7, 2024

View reviewed changes

pullapprove bot requested review from alxhub, AndrewKushnir and thePunderWoman May 7, 2024 23:01

AndrewKushnir approved these changes May 7, 2024

View reviewed changes

pullapprove bot requested a review from atscott May 7, 2024 23:02

thePunderWoman approved these changes May 8, 2024

View reviewed changes

iteriani removed request for atscott and alxhub May 8, 2024 16:15

pullapprove bot requested review from alxhub and thePunderWoman May 8, 2024 16:15

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 8, 2024

thePunderWoman reviewed May 8, 2024

View reviewed changes

pullapprove bot requested a review from thePunderWoman May 8, 2024 16:29

thePunderWoman approved these changes May 8, 2024

View reviewed changes

thePunderWoman removed the request for review from alxhub May 8, 2024 16:41

atscott pushed a commit that referenced this pull request May 9, 2024

refactor(core): Export some more symbols and check for truthiness on …

d00f9e8

…event types before adding them. (#55587) In some cases, we will be passing in undefined for capture events, so handle this. PR Close #55587

atscott closed this in f5b6b7f May 9, 2024

This was referenced May 9, 2024

refactor(core) Remove enums from event-dispatch. #55421

Closed

refactor(core): Remove custom event and replay behavior. #55695

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Use the early event contract instead of the event contract in the bootstrap. #55587

refactor(core): Use the early event contract instead of the event contract in the bootstrap. #55587

iteriani commented Apr 30, 2024

AndrewKushnir left a comment

tbondwilkinson left a comment

iteriani commented May 6, 2024

AndrewKushnir commented May 7, 2024

AndrewKushnir left a comment

AndrewKushnir May 7, 2024

AndrewKushnir left a comment

thePunderWoman left a comment

iteriani commented May 8, 2024

thePunderWoman May 8, 2024

iteriani May 8, 2024

thePunderWoman left a comment

iteriani commented May 8, 2024

atscott commented May 9, 2024

angular-automatic-lock-bot bot commented Jun 10, 2024

refactor(core): Use the early event contract instead of the event contract in the bootstrap. #55587

refactor(core): Use the early event contract instead of the event contract in the bootstrap. #55587

Conversation

iteriani commented Apr 30, 2024

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

AndrewKushnir left a comment

Choose a reason for hiding this comment

tbondwilkinson left a comment

Choose a reason for hiding this comment

iteriani commented May 6, 2024

AndrewKushnir commented May 7, 2024

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir May 7, 2024

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

thePunderWoman left a comment

Choose a reason for hiding this comment

iteriani commented May 8, 2024

thePunderWoman May 8, 2024

Choose a reason for hiding this comment

iteriani May 8, 2024

Choose a reason for hiding this comment

thePunderWoman left a comment

Choose a reason for hiding this comment

iteriani commented May 8, 2024

atscott commented May 9, 2024

angular-automatic-lock-bot bot commented Jun 10, 2024