refactor(core): Allow the container and the listenable element to be … #55586

iteriani · 2024-04-30T00:15:15Z

…configurable for early event contract.

This will allow a multi-app application to listen to early events from different elements and place them on a separate field on the window.

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

tbondwilkinson

Let's fix capture events too.

The smallest way is maybe

addEvents(types: string[], capture = false)

You'll need to also add an extra property for event types that are capture

i can't respond but i did update

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

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

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

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

packages/core/primitives/event-dispatch/test/eventcontract_test.ts

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

packages/core/primitives/event-dispatch/test/eventcontract_test.ts

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

comments all resolved

tbondwilkinson

Last comment, otherwise things look good

tbondwilkinson · 2024-04-30T20:23:51Z

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

@@ -9,7 +9,7 @@
 import {createEventInfoFromParameters, EventInfo} from './event_info';

 declare global {
- interface Window {
+ interface Object {


It's still weird to add this to the global Object.

I think you should do:

export declare interface EarlyJsactionDataContainer { _ejsa?: EarlyJsactionData; } constructor(private readonly earlyJsactionDataContainer: EarlyJsactionDataContainer = window, private readonly container = window.document.documentElement) { this.earlyJsactionDataContainer._ejsa = ...; } replayEarlyEvents(earlyJsactionDataContainer: EarlyJsactionDataContainer = window) {}

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

AndrewKushnir · 2024-05-01T01:21:52Z

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

+ const eventType: string = earlyEventTypes[idx];
+ earlyJsactionData.c.removeEventListener(eventType, earlyEventHandler);
+ }
+ delete earlyJsactionContainer._ejsa;


This is a pre-existing code, but I wanted to mention that we generally try not to use delete operations to avoid potential performance penalties related to deopt in hidden classes in V8. We can set the value to undefined instead. Since this is pre-existing, we can do it in a separate PR.

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

AndrewKushnir · 2024-05-01T01:30:01Z

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

+ }
+ earlyJsactionTracker[field][appId] = {};


Can we do a tree-shakable dev-mode checks in this code? If yes, it might be great to assert whether earlyJsactionTracker[field][appId] already exists and throw an error (otherwise, we just override what we've stored before).

there's no such thing as a dev mode binary i think...

I think we do have some dev-mode flags 1P/3P compatible (via copybara transform) in the signals code.

We can also do this in a followup PR (let's not block this PR on this change).

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

…configurable for early event contract. This will allow a multi-app application to listen to early events from different elements and place them on a separate field on the window.

… to be configurable for early event contract.

AndrewKushnir

@iteriani looks great, just a couple minor refactoring proposals.

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

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

… to be configurable for early event contract.

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

packages/core/primitives/event-dispatch/index.ts

… to be configurable for early event contract.

AndrewKushnir

@iteriani thanks for addressing all the feedback 👍

AndrewKushnir · 2024-05-02T19:16:54Z

goldens/public-api/core/primitives/event-dispatch/index.md

@@ -5,7 +5,7 @@
 ```ts

 // @public
-export function bootstrapEventContract(field: string, container: Element, appId: string, events: string[], anyWindow?: any): EventContract;
+export function bootstrapEventContract(field: string, container: Element, appId: string, events: string[], earlyJsactionTracker?: any): void;


You'll probably need to re-generate golden files, this any would be replaced with Tracker type.

thePunderWoman

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

… to be configurable for early event contract.

iteriani · 2024-05-02T19:57:26Z

Caretaker note: TGP is clean, so this is safe to merge

… to be configurable for early event contract.

AndrewKushnir

Reviewed-for: public-api

iteriani · 2024-05-03T15:14:06Z

TGP is clean, so this is safe to merge

AndrewKushnir · 2024-05-03T18:26:07Z

TESTED=TGP (copy from #55586 (comment))

AndrewKushnir · 2024-05-03T18:27:00Z

Caretaker note: the merge is scheduled for Monday, 05/06.

AndrewKushnir · 2024-05-06T15:15:57Z

This PR was merged into the repository by commit 8f273ce.

…configurable for early event contract. (#55586) This will allow a multi-app application to listen to early events from different elements and place them on a separate field on the window. PR Close #55586

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

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

iteriani force-pushed the earlyeventcontract branch 3 times, most recently from bd8cfd2 to c847dd5 Compare April 30, 2024 00:20

thePunderWoman added the area: core Issues related to the framework runtime label Apr 30, 2024

ngbot bot added this to the Backlog milestone Apr 30, 2024

thePunderWoman added action: review The PR is still awaiting reviews from at least one requested reviewer target: rc This PR is targeted for the next release-candidate labels Apr 30, 2024

tbondwilkinson previously requested changes Apr 30, 2024

View reviewed changes

iteriani mentioned this pull request Apr 30, 2024

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

Closed

14 tasks

iteriani requested a review from tbondwilkinson April 30, 2024 17:56

tbondwilkinson requested changes Apr 30, 2024

View reviewed changes

tbondwilkinson previously requested changes Apr 30, 2024

View reviewed changes

tbondwilkinson requested changes Apr 30, 2024

View reviewed changes

tbondwilkinson approved these changes Apr 30, 2024

View reviewed changes

iteriani force-pushed the earlyeventcontract branch 2 times, most recently from f89ade7 to ee62a89 Compare April 30, 2024 21:28

AndrewKushnir reviewed May 1, 2024

View reviewed changes

iteriani added 6 commits May 1, 2024 13:32

refactor(core): Allow the container and the listenable element to be …

6af29b0

…configurable for early event contract. This will allow a multi-app application to listen to early events from different elements and place them on a separate field on the window.

fixup! refactor(core): Allow the container and the listenable element…

ba01a42

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

b406441

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

c2bbd97

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

c786d8a

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

6596a07

… to be configurable for early event contract.

iteriani force-pushed the earlyeventcontract branch from ee62a89 to a77d9d5 Compare May 1, 2024 20:50

iteriani requested a review from AndrewKushnir May 1, 2024 22:54

iteriani added 2 commits May 1, 2024 20:15

fixup! refactor(core): Allow the container and the listenable element…

6fc5cf1

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

83cb5f5

… to be configurable for early event contract.

AndrewKushnir reviewed May 2, 2024

View reviewed changes

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

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

iteriani added 3 commits May 2, 2024 11:04

fixup! refactor(core): Allow the container and the listenable element…

bd8ff2c

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

db26ee0

… to be configurable for early event contract.

fixup! refactor(core): Allow the container and the listenable element…

11e95a0

… to be configurable for early event contract.

AndrewKushnir reviewed May 2, 2024

View reviewed changes

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

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

fixup! refactor(core): Allow the container and the listenable element…

2726fc7

… to be configurable for early event contract.

AndrewKushnir approved these changes May 2, 2024

View reviewed changes

pullapprove bot requested review from AndrewKushnir, atscott and thePunderWoman May 2, 2024 19:17

thePunderWoman approved these changes May 2, 2024

View reviewed changes

pullapprove bot requested a review from pkozlowski-opensource May 2, 2024 19:21

fixup! refactor(core): Allow the container and the listenable element…

7e6d966

… to be configurable for early event contract.

iteriani 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 2, 2024

fixup! refactor(core): Allow the container and the listenable element…

e8159de

… to be configurable for early event contract.

AndrewKushnir approved these changes May 2, 2024

View reviewed changes

AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label May 2, 2024

iteriani removed request for atscott and pkozlowski-opensource May 3, 2024 15:04

iteriani added the action: merge The PR is ready for merge by the caretaker label May 3, 2024

AndrewKushnir closed this in 8f273ce May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Allow the container and the listenable element to be … #55586

refactor(core): Allow the container and the listenable element to be … #55586

iteriani commented Apr 30, 2024

tbondwilkinson left a comment

tbondwilkinson left a comment

tbondwilkinson Apr 30, 2024

iteriani Apr 30, 2024

AndrewKushnir May 1, 2024

AndrewKushnir May 1, 2024

iteriani May 1, 2024

AndrewKushnir May 2, 2024

AndrewKushnir May 2, 2024

AndrewKushnir left a comment

AndrewKushnir left a comment

AndrewKushnir May 2, 2024

thePunderWoman left a comment

iteriani commented May 2, 2024 •

edited

AndrewKushnir left a comment

iteriani commented May 3, 2024 •

edited

AndrewKushnir commented May 3, 2024 •

edited

AndrewKushnir commented May 3, 2024

AndrewKushnir commented May 6, 2024

refactor(core): Allow the container and the listenable element to be … #55586

refactor(core): Allow the container and the listenable element to be … #55586

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

tbondwilkinson left a comment

Choose a reason for hiding this comment

tbondwilkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thePunderWoman left a comment

Choose a reason for hiding this comment

iteriani commented May 2, 2024 • edited

AndrewKushnir left a comment

Choose a reason for hiding this comment

iteriani commented May 3, 2024 • edited

AndrewKushnir commented May 3, 2024 • edited

AndrewKushnir commented May 3, 2024

AndrewKushnir commented May 6, 2024

iteriani commented May 2, 2024 •

edited

iteriani commented May 3, 2024 •

edited

AndrewKushnir commented May 3, 2024 •

edited