-
Notifications
You must be signed in to change notification settings - Fork 25k
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.
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
refactor(core): Rename BaseDispatcher
to Dispatcher
.
#55721
refactor(core): Rename BaseDispatcher
to Dispatcher
.
#55721
Conversation
476051f
to
cdcbe6b
Compare
TESTED=TGP |
setEventReplayer(eventReplayer: Replayer) { | ||
this.eventReplayer = eventReplayer; | ||
this.eventReplayScheduled = true; | ||
Promise.resolve().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why do we replay in a microtask? Would it make sense to add a comment here?
I was also thinking if we can use queueMicrotask
here instead. The code is pre-existing, so we can handle it in a separate PR (so that this PR retains an existing logic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queueMicrotask
is not yet in CDW tier1, https://developer.mozilla.org/en-US/docs/Web/API/queueMicrotask
This microtask logic predates our efforts here, I suspect we can get rid of it, but it has a high risk of rollback due to being a breaking timing change, so we'd want to let that simmer as a single change PR, perhaps even with a easy rollback mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This microtask logic predates our efforts here, I suspect we can get rid of it
Thanks for additional info. I'm not proposing to remove it, just curious why we do it. If we have any info - we can add as a comment to the code, if we don't have this info (and it's there for historical reason) - we can add a comment later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only information I have is that comment - that it "emulates" browser behavior. I suspect this is more to do with the legacy behavior of the dispatcher, which is to schedule event replay whenever a new handler is registered. We can preserve that behavior in the LegacyDispatcher though.
cdcbe6b
to
4f2436c
Compare
There was a problem hiding this 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
c1f95f3
to
a6f024f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Reviewed-for: public-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed-for: fw-core, primitives, public-api
Rename `BaseDispatcher` to `Dispatcher` and `Dispatcher` to `LegacyDispatcher`. The `GlobalHandler` type and `stopPropagation` function needs to be left for now in dispatcher.ts as it was not exported previously from legacy_dispatcher.ts.
a6f024f
to
047ee79
Compare
This PR was merged into the repository by commit 0cb5031. |
Rename `BaseDispatcher` to `Dispatcher` and `Dispatcher` to `LegacyDispatcher`. The `GlobalHandler` type and `stopPropagation` function needs to be left for now in dispatcher.ts as it was not exported previously from legacy_dispatcher.ts. PR Close #55721
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Rename
BaseDispatcher
toDispatcher
andDispatcher
toLegacyDispatcher
. TheGlobalHandler
type needs to be left for now in dispatcher.ts as it was not exported previously from legacy_dispatcher.ts.PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?