Skip to content

Reduce the handleTopLevel() event code indirection #11915

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.

Sign up for GitHub

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

Merged
gaearon merged 3 commits into from
Jan 5, 2018
Merged

Reduce the handleTopLevel() event code indirection #11915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f72f16a
Refactor event emitters to reduce indirection
gaearon Dec 22, 2017
646d081
Remove unused handleTopLevel() injection
gaearon Dec 22, 2017
034775b
Rename handleTopLevel() to runExtractedEventsInBatch() and remove imp…
gaearon Dec 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 22 additions & 20 deletions packages/events/EventPluginHub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,13 +164,13 @@ export function getListener(inst: Fiber, registrationName: string) {
* @return {*} An accumulation of synthetic events.
* @internal
*/
export function extractEvents(
function extractEvents(
topLevelType: string,
targetInst: Fiber,
nativeEvent: AnyNativeEvent,
nativeEventTarget: EventTarget,
) {
let events;
): Array<ReactSyntheticEvent> | ReactSyntheticEvent | null {
let events = null;
for (let i = 0; i < plugins.length; i++) {
// Not every plugin in the ordering may be loaded at runtime.
const possiblePlugin: PluginModule<AnyNativeEvent> = plugins[i];
Expand All @@ -189,27 +189,14 @@ export function extractEvents(
return events;
}

/**
* Enqueues a synthetic event that should be dispatched when
* `processEventQueue` is invoked.
*
* @param {*} events An accumulation of synthetic events.
* @internal
*/
export function enqueueEvents(
events: Array<ReactSyntheticEvent> | ReactSyntheticEvent,
export function runEventsInBatch(
Copy link
Collaborator Author

@gaearon gaearon Dec 22, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@nhunzaker nhunzaker Jan 4, 2018

Choose a reason for hiding this comment

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

I think it's fine, and much clearer.

events: Array<ReactSyntheticEvent> | ReactSyntheticEvent | null,
simulated: boolean,
) {
if (events) {
if (events !== null) {
eventQueue = accumulateInto(eventQueue, events);
}
}

/**
* Dispatches all synthetic events on the event queue.
*
* @internal
*/
export function processEventQueue(simulated: boolean) {
// Set `eventQueue` to null before processing it so that we can tell if more
// events get enqueued while processing.
const processingEventQueue = eventQueue;
Expand Down Expand Up @@ -238,3 +225,18 @@ export function processEventQueue(simulated: boolean) {
// This would be a good time to rethrow if any of the event handlers threw.
ReactErrorUtils.rethrowCaughtError();
}

export function runExtractedEventsInBatch(
topLevelType: string,
targetInst: Fiber,
nativeEvent: AnyNativeEvent,
nativeEventTarget: EventTarget,
) {
const events = extractEvents(
topLevelType,
targetInst,
nativeEvent,
nativeEventTarget,
);
runEventsInBatch(events, false);
}
36 changes: 0 additions & 36 deletions packages/events/ReactEventEmitterMixin.js
View file
Open in desktop

This file was deleted.

3 changes: 1 addition & 2 deletions packages/events/__tests__/ResponderEventPlugin-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,8 +312,7 @@ const run = function(config, hierarchyConfig, nativeEventConfig) {
// At this point the negotiation events have been dispatched as part of the
// extraction process, but not the side effectful events. Below, we dispatch
// side effectful events.
EventPluginHub.enqueueEvents(extractedEvents);
EventPluginHub.processEventQueue(true);
EventPluginHub.runEventsInBatch(extractedEvents, true);

// Ensure that every event that declared an `order`, was actually dispatched.
expect('number of events dispatched:' + runData.dispatchCount).toBe(
Expand Down
4 changes: 0 additions & 4 deletions packages/react-dom/src/client/ReactDOMClientInjection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,9 @@ import BeforeInputEventPlugin from '../events/BeforeInputEventPlugin';
import ChangeEventPlugin from '../events/ChangeEventPlugin';
import DOMEventPluginOrder from '../events/DOMEventPluginOrder';
import EnterLeaveEventPlugin from '../events/EnterLeaveEventPlugin';
import {handleTopLevel} from '../events/ReactBrowserEventEmitter';
import {setHandleTopLevel} from '../events/ReactDOMEventListener';
import SelectEventPlugin from '../events/SelectEventPlugin';
import SimpleEventPlugin from '../events/SimpleEventPlugin';

setHandleTopLevel(handleTopLevel);

/**
* Inject modules for resolving DOM hierarchy and plugin ordering.
*/
Expand Down
5 changes: 2 additions & 3 deletions packages/react-dom/src/events/ChangeEventPlugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

@nhunzaker nhunzaker Jan 4, 2018

Choose a reason for hiding this comment

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

Why glob this instead of import { runEventsInBatch } from 'events/EventPluginHub'?

import {accumulateTwoPhaseDispatches} from 'events/EventPropagators';
import {enqueueStateRestore} from 'events/ReactControlledComponent';
import {batchedUpdates} from 'events/ReactGenericBatching';
Expand Down Expand Up @@ -89,8 +89,7 @@ function manualDispatchChangeEvent(nativeEvent) {
}

function runEventInBatch(event) {
enqueueEvents(event);
processEventQueue(false);
EventPluginHub.runEventsInBatch(event, false);
}

function getInstIfValueChanged(targetInst) {
Expand Down
3 changes: 0 additions & 3 deletions packages/react-dom/src/events/ReactBrowserEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
*/

import {registrationNameDependencies} from 'events/EventPluginRegistry';

import {
setEnabled,
isEnabled,
Expand All @@ -16,8 +15,6 @@ import {
import isEventSupported from './isEventSupported';
import BrowserEventConstants from './BrowserEventConstants';

export * from 'events/ReactEventEmitterMixin';

const {topLevelTypes} = BrowserEventConstants;

/**
Expand Down
12 changes: 4 additions & 8 deletions packages/react-dom/src/events/ReactDOMEventListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import {batchedUpdates} from 'events/ReactGenericBatching';
import {runExtractedEventsInBatch} from 'events/EventPluginHub';
import {isFiberMounted} from 'react-reconciler/reflection';
import {HostRoot} from 'shared/ReactTypeOfWork';

Expand Down Expand Up @@ -62,7 +63,7 @@ function releaseTopLevelCallbackBookKeeping(instance) {
}
}

function handleTopLevelImpl(bookKeeping) {
function handleTopLevel(bookKeeping) {
let targetInst = bookKeeping.targetInst;

// Loop through the hierarchy, in case there's any nested components.
Expand All @@ -85,7 +86,7 @@ function handleTopLevelImpl(bookKeeping) {

for (let i = 0; i < bookKeeping.ancestors.length; i++) {
targetInst = bookKeeping.ancestors[i];
_handleTopLevel(
runExtractedEventsInBatch(
bookKeeping.topLevelType,
targetInst,
bookKeeping.nativeEvent,
Expand All @@ -96,11 +97,6 @@ function handleTopLevelImpl(bookKeeping) {

// TODO: can we stop exporting these?
export let _enabled = true;
export let _handleTopLevel: null;

export function setHandleTopLevel(handleTopLevel) {
_handleTopLevel = handleTopLevel;
}

export function setEnabled(enabled) {
_enabled = !!enabled;
Expand Down Expand Up @@ -180,7 +176,7 @@ export function dispatchEvent(topLevelType, nativeEvent) {
try {
// Event queue being processed in the same cycle allows
// `preventDefault`.
batchedUpdates(handleTopLevelImpl, bookKeeping);
batchedUpdates(handleTopLevel, bookKeeping);
} finally {
releaseTopLevelCallbackBookKeeping(bookKeeping);
}
Expand Down
4 changes: 1 addition & 3 deletions packages/react-dom/src/test-utils/ReactTestUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,7 @@ function makeSimulator(eventType) {
// Normally extractEvent enqueues a state restore, but we'll just always
// do that since we we're by-passing it here.
ReactControlledComponent.enqueueStateRestore(domNode);

EventPluginHub.enqueueEvents(event);
EventPluginHub.processEventQueue(true);
EventPluginHub.runEventsInBatch(event, true);
});
};
}
Expand Down
19 changes: 12 additions & 7 deletions packages/react-native-renderer/src/ReactNativeEventEmitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,16 @@
* @flow
*/

import {getListener} from 'events/EventPluginHub';
import {getListener, runExtractedEventsInBatch} from 'events/EventPluginHub';
import {registrationNameModules} from 'events/EventPluginRegistry';
import {batchedUpdates} from 'events/ReactGenericBatching';
import {handleTopLevel} from 'events/ReactEventEmitterMixin';
import warning from 'fbjs/lib/warning';

import {getInstanceFromNode} from './ReactNativeComponentTree';
import ReactNativeTagHandles from './ReactNativeTagHandles';

export * from 'events/ReactEventEmitterMixin';
import type {AnyNativeEvent} from 'events/PluginModuleType';

export {getListener, registrationNameModules as registrationNames};

/**
Expand All @@ -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);
Copy link
Collaborator Author

@gaearon gaearon Dec 22, 2017

Choose a reason for hiding this comment

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

This is a bit shady. I actually don't know why it's necessary at all. Native events don't always have event objects?

Copy link
Contributor

@jquense jquense Dec 22, 2017

Choose a reason for hiding this comment

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

not that I've ever seen

Copy link
Collaborator Author

@gaearon gaearon Dec 22, 2017

Choose a reason for hiding this comment

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

(This is specific to React Native)


/**
* Selects a subsequence of `Touch`es, without destroying `touches`.
Expand Down Expand Up @@ -90,12 +90,17 @@ const removeTouchesAtIndices = function(
export function _receiveRootNodeIDEvent(
rootNodeID: number,
topLevelType: string,
nativeEventParam: ?Object,
nativeEventParam: ?AnyNativeEvent,
) {
const nativeEvent = nativeEventParam || EMPTY_NATIVE_EVENT;
const inst = getInstanceFromNode(rootNodeID);
batchedUpdates(function() {
handleTopLevel(topLevelType, inst, nativeEvent, nativeEvent.target);
runExtractedEventsInBatch(
topLevelType,
inst,
nativeEvent,
nativeEvent.target,
);
});
// React Native doesn't use ReactControlledComponent but if it did, here's
// where it would do it.
Expand All @@ -111,7 +116,7 @@ export function _receiveRootNodeIDEvent(
export function receiveEvent(
rootNodeID: number,
topLevelType: string,
nativeEventParam: Object,
nativeEventParam: AnyNativeEvent,
) {
_receiveRootNodeIDEvent(rootNodeID, topLevelType, nativeEventParam);
}
Expand Down