Reduce the handleTopLevel() event code indirection #11915
Conversation
gaearon
changed the title
|
I forgot to edit PR title from the branch name when submitting 😄 Embarrassing. |
| @@ -25,7 +25,7 @@ export {getListener, registrationNameModules as registrationNames}; | |||
| */ | |||
|
|
|||
| // Shared default empty native event - conserve memory. | |||
| const EMPTY_NATIVE_EVENT = {}; | |||
| const EMPTY_NATIVE_EVENT = (({}: any): AnyNativeEvent); | |||
There was a problem hiding this comment.
This is a bit shady. I actually don't know why it's necessary at all. Native events don't always have event objects?
There was a problem hiding this comment.
(This is specific to React Native)
| */ | ||
| export function enqueueEvents( | ||
| events: Array<ReactSyntheticEvent> | ReactSyntheticEvent, | ||
| export function runEventsInBatch( |
There was a problem hiding this comment.
Happy to bikeshed on naming here. On the one hand it's not exactly clear this is flushing an existing queue as well. On the other hand, as far as I can tell, it's not supported to enqueue more events while we're extracting them (there's an invariant below saying it's not implemented). So the naming is probably fine since it's not supported to enqueue and not flush them anyway.
There was a problem hiding this comment.
I think it's fine, and much clearer.
| @@ -5,7 +5,7 @@ | |||
| * LICENSE file in the root directory of this source tree. | |||
| */ | |||
|
|
|||
| import {enqueueEvents, processEventQueue} from 'events/EventPluginHub'; | |||
| import * as EventPluginHub from 'events/EventPluginHub'; | |||
There was a problem hiding this comment.
Why glob this instead of import { runEventsInBatch } from 'events/EventPluginHub'?
|
@gaearon aside from approval, anything else I can do to help move this along? |
* Refactor event emitters to reduce indirection * Remove unused handleTopLevel() injection * Rename handleTopLevel() to runExtractedEventsInBatch() and remove import indirection
* Refactor event emitters to reduce indirection * Remove unused handleTopLevel() injection * Rename handleTopLevel() to runExtractedEventsInBatch() and remove import indirection
This doesn't measurably change anything, but makes the code clearer in my opinion.
Previously we had
ReactEventEmitterMixinthat exported a singlehandleTopLevel()helper. This function was re-exported from "specific" modules likeReactBrowserEventEmitterandReactNativeEventEmitter. The "mixin" naming refers to it previously beingObject.assign-ed, although it's not the case anymore since we migrated to ES Modules.I noticed that the mixin is extremely tiny and just calls two functions on
EventPluginHub. At first I wanted to inline it into the "specific" emitters, but then I realized I might just put it intoEventPluginHub. This got me thinking: why do we even exposeenqueueEvents()andprocessEventQueue()fromEventPluginHub? Because we use them fromhandleTopLevel()and a few other places. But now thathandleTopLevel()is inEventPluginHub, can we unify other callers behind a single method too? It turns out that they are always called one after another, so they might as well be unified into a single method. I did just that, and called itrunEventsInBatch().In the second commit, I realized that there is no need to inject
handleTopLevel()dynamically because it's always known statically, and we no longer rely on overriding it from tests (fixed in #11327). So I hardcoded it instead.In the third commit, I renamed
handleTopLevel()torunExtractedEventsInBatch()to make it clear that it is equivalent torunEventsInBatch()for the extracted events. I also removed a bunch of module redirects, and changed the callers to import it fromEventPluginHubdirectly since neither of the redirect sites actually did anything with the arguments.