Skip to content
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

Jump to bottom

Do not bind topLevelType to dispatch #13618

Merged
merged 3 commits into from
Sep 14, 2018
Merged

Do not bind topLevelType to dispatch #13618

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
7d2b2f1
Do not bind topLevelType to dispatch
nhunzaker Sep 11, 2018
4e1f1a6
Add note about why casting event.type to a topLevelType is safe
nhunzaker Sep 11, 2018
2581987
Move interactiveUpdates comment to point of assignment
nhunzaker Sep 11, 2018
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
2 changes: 1 addition & 1 deletion packages/events/PluginModuleType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type {TopLevelType} from './TopLevelEventTypes';

export type EventTypes = {[key: string]: DispatchConfig};

export type AnyNativeEvent = Event | KeyboardEvent | MouseEvent | Touch;
export type AnyNativeEvent = Event | KeyboardEvent | MouseEvent | TouchEvent;
Copy link
Contributor Author

@nhunzaker nhunzaker Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably go straight to master. As far as I can tell, this is a bug, but I don't know if Touch is defined somewhere else or is actually correctly referencing the Touch DOM constructor


export type PluginName = string;

Expand Down
8 changes: 4 additions & 4 deletions packages/events/ReactGenericBatching.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ import {
let _batchedUpdatesImpl = function(fn, bookkeeping) {
return fn(bookkeeping);
};
let _interactiveUpdatesImpl = function(fn, a, b) {
return fn(a, b);
let _interactiveUpdatesImpl = function(fn, a) {
return fn(a);
};
let _flushInteractiveUpdatesImpl = function() {};

Expand Down Expand Up @@ -52,8 +52,8 @@ export function batchedUpdates(fn, bookkeeping) {
}
}

export function interactiveUpdates(fn, a, b) {
return _interactiveUpdatesImpl(fn, a, b);
export function interactiveUpdates(fn, a) {
return _interactiveUpdatesImpl(fn, a);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This possibly affects other renders 😨

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you checked whether it does? ReactGenericBatching is only accessible in this repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should not affect other renders. ReactGenericBatching.interactiveUpdates is only used inside of ReactDOMEventListener.


export function flushInteractiveUpdates() {
Expand Down
38 changes: 14 additions & 24 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 @@ -21,6 +21,7 @@ import getEventTarget from './getEventTarget';
import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree';
import SimpleEventPlugin from './SimpleEventPlugin';
import {getRawEventName} from './DOMTopLevelEventTypes';
import {unsafeCastStringToDOMTopLevelType} from 'events/TopLevelEventTypes';

const {isInteractiveTopLevelEventType} = SimpleEventPlugin;

Expand Down Expand Up @@ -48,7 +49,6 @@ function findRootContainerNode(inst) {

// Used to store ancestor hierarchy in top level callback
function getTopLevelCallbackBookKeeping(
topLevelType,
nativeEvent,
targetInst,
): {
Expand All @@ -57,6 +57,9 @@ function getTopLevelCallbackBookKeeping(
targetInst: Fiber | null,
ancestors: Array<Fiber>,
} {
// This is safe because DOMTopLevelTypes are always native event type strings
const topLevelType = unsafeCastStringToDOMTopLevelType(nativeEvent.type);

if (callbackBookkeepingPool.length) {
const instance = callbackBookkeepingPool.pop();
instance.topLevelType = topLevelType;
Expand Down Expand Up @@ -141,16 +144,13 @@ export function trapBubbledEvent(
if (!element) {
return null;
}

// Check if interactive and wrap in interactiveUpdates
const dispatch = isInteractiveTopLevelEventType(topLevelType)
? dispatchInteractiveEvent
: dispatchEvent;

addEventBubbleListener(
element,
getRawEventName(topLevelType),
// Check if interactive and wrap in interactiveUpdates
dispatch.bind(null, topLevelType),
);
addEventBubbleListener(element, getRawEventName(topLevelType), dispatch);
}

/**
Expand All @@ -169,26 +169,20 @@ export function trapCapturedEvent(
if (!element) {
return null;
}

// Check if interactive and wrap in interactiveUpdates
const dispatch = isInteractiveTopLevelEventType(topLevelType)
? dispatchInteractiveEvent
: dispatchEvent;

addEventCaptureListener(
element,
getRawEventName(topLevelType),
// Check if interactive and wrap in interactiveUpdates
dispatch.bind(null, topLevelType),
);
addEventCaptureListener(element, getRawEventName(topLevelType), dispatch);
}

function dispatchInteractiveEvent(topLevelType, nativeEvent) {
interactiveUpdates(dispatchEvent, topLevelType, nativeEvent);
function dispatchInteractiveEvent(nativeEvent) {
interactiveUpdates(dispatchEvent, nativeEvent);
}

export function dispatchEvent(
topLevelType: DOMTopLevelEventType,
nativeEvent: AnyNativeEvent,
) {
export function dispatchEvent(nativeEvent: AnyNativeEvent) {
if (!_enabled) {
return;
}
Expand All @@ -207,11 +201,7 @@ export function dispatchEvent(
targetInst = null;
}

const bookKeeping = getTopLevelCallbackBookKeeping(
topLevelType,
nativeEvent,
targetInst,
);
const bookKeeping = getTopLevelCallbackBookKeeping(nativeEvent, targetInst);

try {
// Event queue being processed in the same cycle allows
Expand Down
6 changes: 4 additions & 2 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 @@ -42,7 +42,9 @@ const [
runEventsInBatch,
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

function Event(suffix) {}
function Event(type) {
this.type = type;
Copy link
Contributor

Choose a reason for hiding this comment

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

I‘m a bit confused about this - where do we use this event constructor?

Copy link
Contributor Author

@nhunzaker nhunzaker Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

RestTestUtils.SimulateNative:

react/packages/react-dom/src/test-utils/ReactTestUtils.js

Lines 479 to 498 in f6fb03e

function makeNativeSimulator(eventType, topLevelType) {
return function(domComponentOrNode, nativeEventData) {
const fakeNativeEvent = new Event(eventType);
Object.assign(fakeNativeEvent, nativeEventData);
if (ReactTestUtils.isDOMComponent(domComponentOrNode)) {
simulateNativeEventOnDOMComponent(
topLevelType,
domComponentOrNode,
fakeNativeEvent,
);
} else if (domComponentOrNode.tagName) {
// Will allow on actual dom nodes.
simulateNativeEventOnNode(
topLevelType,
domComponentOrNode,
fakeNativeEvent,
);
}
};
}

}

let hasWarnedAboutDeprecatedMockComponent = false;

Expand All @@ -59,7 +61,7 @@ let hasWarnedAboutDeprecatedMockComponent = false;
*/
function simulateNativeEventOnNode(topLevelType, node, fakeNativeEvent) {
fakeNativeEvent.target = node;
dispatchEvent(topLevelType, fakeNativeEvent);
dispatchEvent(fakeNativeEvent);
}

/**
Expand Down