Skip to content

refactor(core): Refactor parts of event_replay into a shared library and move stashing function to listening code#56172

Closed
iteriani wants to merge 1 commit intoangular:mainfrom
iteriani:refactor_event_replay
Closed

refactor(core): Refactor parts of event_replay into a shared library and move stashing function to listening code#56172
iteriani wants to merge 1 commit intoangular:mainfrom
iteriani:refactor_event_replay

Conversation

@iteriani
Copy link
Contributor

@iteriani iteriani commented May 29, 2024

This also moves the code that stashes the jsaction more closely to the code that actually sets the event listener. I've considered just moving that code to the listener itself..but it's in a different package. Maybe we need to add another DomEventsPlugin implementation?

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

@iteriani iteriani requested a review from AndrewKushnir May 29, 2024 23:59
AndrewKushnir
AndrewKushnir reviewed May 30, 2024
View reviewed changes
@thePunderWoman thePunderWoman added the area: core Issues related to the framework runtime label May 30, 2024
@ngbot ngbot bot added this to the Backlog milestone May 30, 2024
@iteriani iteriani force-pushed the refactor_event_replay branch from 89bf6c9 to db58abd Compare May 30, 2024 18:27
@pullapprove pullapprove bot requested a review from tbondwilkinson May 30, 2024 18:27
@pullapprove pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label May 30, 2024
@iteriani iteriani force-pushed the refactor_event_replay branch 3 times, most recently from 7e02127 to 7ffcd83 Compare May 30, 2024 21:09
tbondwilkinson
tbondwilkinson approved these changes May 30, 2024
View reviewed changes
@iteriani iteriani force-pushed the refactor_event_replay branch 2 times, most recently from dcde347 to 03c8f94 Compare May 30, 2024 22:14
AndrewKushnir
AndrewKushnir reviewed May 31, 2024
View reviewed changes
thePunderWoman
thePunderWoman reviewed May 31, 2024
View reviewed changes
@iteriani iteriani force-pushed the refactor_event_replay branch 2 times, most recently from 0e8abdd to b9555dc Compare June 4, 2024 00:31
AndrewKushnir
AndrewKushnir reviewed Jun 4, 2024
View reviewed changes
packages/core/src/event_delegation_utils.ts Outdated
Comment on lines 104 to 84
Copy link
Contributor

@AndrewKushnir AndrewKushnir Jun 4, 2024

Choose a reason for hiding this comment

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

I think we can avoid referencing IS_EVENT_REPLAY_ENABLED here and instead check if the eventDelegation.eventContract is defined:

Suggested change
export const initGlobalEventDelegation = (
eventDelegation: GlobalEventDelegation,
injector: Injector,
) => {
if (injector.get(IS_EVENT_REPLAY_ENABLED, EVENT_REPLAY_ENABLED_DEFAULT)) {
return;
}
const dispatcher = new EventDispatcher(invokeRegisteredListeners);
registerDispatcher(eventDelegation.eventContract, dispatcher);
};
export const initGlobalEventDelegation = (
eventDelegation: GlobalEventDelegation,
eventContractContainer: Element,
) => {
if (eventDelegation.eventContract) {
// EventContract is already configured for this application,
// no need to create a new one, exit.
return;
}
const container = new EventContractContainer(eventContractContainer);
const eventContract = new EventContract(container);
const dispatcher = new EventDispatcher(invokeRegisteredListeners);
registerDispatcher(eventContract, dispatcher);
eventDelegation.eventContract = eventContract;
};

I've also updated the function to init EventContract if needed. We can use this function later in the client-only code path. WDYT?

Copy link
Contributor Author

@iteriani iteriani Jun 4, 2024

Choose a reason for hiding this comment

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

i was a little afraid of order of operations in the case both eventReplay() and withGlobalEventDelegation() are available. I did initialize the event contract though.

@iteriani iteriani force-pushed the refactor_event_replay branch 4 times, most recently from 8e56dc1 to eb46d58 Compare June 4, 2024 18:30
thePunderWoman
thePunderWoman approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@thePunderWoman thePunderWoman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

I'm comfortable with these changes after some discussion with @iteriani.

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

AndrewKushnir
AndrewKushnir approved these changes Jun 4, 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.

LGTM, just one comment with a refactoring proposal.

AndrewKushnir
AndrewKushnir approved these changes Jun 4, 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

@iteriani
refactor(core): Refactor parts of event_replay into a shared library …
5406104 
…that will be used with global event delegation.

This also moves the code that stashes the jsaction more closely to the code that actually sets the event listener.
@iteriani iteriani force-pushed the refactor_event_replay branch from eb46d58 to 5406104 Compare June 4, 2024 23:47
@iteriani iteriani added the action: merge The PR is ready for merge by the caretaker label Jun 5, 2024
@ngbot
Copy link

ngbot bot commented Jun 5, 2024

I see that you just added the action: merge label, but the following checks are still failing:
    failure status "google-internal-tests" is failing
    pending missing required labels: target: *
    pending 3 pending code reviews

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken main, please try rebasing to main and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@iteriani
Copy link
Contributor Author

iteriani commented Jun 5, 2024

TESTED=TGP

Failing tests appear unrelated

@thePunderWoman thePunderWoman added target: patch This PR is targeted for the next patch release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jun 5, 2024
thePunderWoman pushed a commit that referenced this pull request Jun 5, 2024
@iteriani @thePunderWoman
refactor(core): Refactor parts of event_replay into a shared library …
dbf1d93 
…that will be used with global event delegation. (#56172)

This also moves the code that stashes the jsaction more closely to the code that actually sets the event listener.

PR Close #56172
@thePunderWoman
Copy link
Contributor

thePunderWoman commented Jun 5, 2024

This PR was merged into the repository by commit 6e89ef1.

@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Jul 6, 2024

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

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

4 participants

@iteriani @thePunderWoman @tbondwilkinson @AndrewKushnir