Do not bind topLevelType to dispatch #13618

nhunzaker · 2018-09-11T14:54:17Z

This PR is exploratory, but it's been on my mind a long time and I wanted to get others' thoughts.

A previous change (#12629) made it such that all top level event types correspond to their associated native event string values. That means we don't need to bind that event name into dispatchEvent, letting all event listener code use either the standard dispatch function, or the variant for interactive updates.

Why

My primary motivation with this PR isn't to necessarily merge this, but to talk about a few event system things.

Why drop bind()

Dropping this .bind() is attractive to me because I believe we could eventually remove the logic inReactDOMBrowserEventEmitter to track what events have already attached:

react/packages/react-dom/src/events/ReactBrowserEventEmitter.js

Lines 95 to 103 in f6fb03e

    
           function getListeningForDocument(mountAt: any) { 
        
             // In IE8, `mountAt` is a host object and doesn't have `hasOwnProperty` 
        
             // directly. 
        
             if (!Object.prototype.hasOwnProperty.call(mountAt, topListenersIDKey)) { 
        
               mountAt[topListenersIDKey] = reactTopListenersCounter++; 
        
               alreadyListeningTo[mountAt[topListenersIDKey]] = {}; 
        
             } 
        
             return alreadyListeningTo[mountAt[topListenersIDKey]]; 
        
           }

react/packages/react-dom/src/events/ReactBrowserEventEmitter.js

Lines 130 to 135 in f6fb03e

    
           const isListening = getListeningForDocument(mountAt); 
        
           const dependencies = registrationNameDependencies[registrationName]; 
        
           for (let i = 0; i < dependencies.length; i++) { 
        
             const dependency = dependencies[i]; 
        
             if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) {

When working on the scroll-jank PR (#9333), tracking needs to happen per-element for touch, wheel, and scroll events. That means saving state on each DOM node, and doing a lot of property access.

Could it be faster?

I'm curious about the performance characteristics of naively attaching the same event. It could avoid a lot of DOM storage and property access (event listener attachment tracking) if we start to attach listeners locally, since addEventListener is idempotent:

let button = document.createElement('button')
let callback = () => console.log("click!")

button.addEventListener('click', callback)
button.addEventListener('click', callback)

button.click()

Challenges

We can't remove event listener tracking because SelectEventPlugin depends on this tracking to see if it should fire:

react/packages/react-dom/src/events/SelectEventPlugin.js

Lines 173 to 177 in f6fb03e

    
           // Track whether all listeners exists for this plugin. If none exist, we do 
        
           // not extract events. See #3639. 
        
           if (!doc || !isListeningToAllDependencies('onSelect', doc)) { 
        
             return null; 
        
           }

But we might be able to come up with another way to determine when SelectEventPlugin should fire. Maybe, instead of enumerating all plugins at dispatch time, they could be eagerly applied on mount. Then we also wouldn't have to run through all event plugins on every dispatch because the event plugins would have the context to know that their behavior was specifically applied.

Still, it's an interesting series of changes to me, and I wanted to get the thoughts of others.

nhunzaker · 2018-09-11T14:58:16Z

packages/events/PluginModuleType.js

@@ -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;


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

nhunzaker · 2018-09-11T14:59:27Z

packages/events/ReactGenericBatching.js

-export function interactiveUpdates(fn, a, b) {
-  return _interactiveUpdatesImpl(fn, a, b);
+export function interactiveUpdates(fn, a) {
+  return _interactiveUpdatesImpl(fn, a);
 }


This possibly affects other renders 😨

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

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

pull-bot · 2018-09-11T15:06:19Z

Details of bundled changes.

Comparing: f6fb03e...16ad8d6

schedule

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
schedule.development.js	n/a	n/a	0 B	19.17 KB	0 B	5.74 KB	UMD_DEV
schedule.production.min.js	n/a	n/a	0 B	3.16 KB	0 B	1.53 KB	UMD_PROD

Generated by 🚫 dangerJS

gaearon · 2018-09-11T15:52:13Z

This by itself seems fine to me and is a bit simpler. What's the concern?

nhunzaker · 2018-09-11T16:31:17Z

@gaearon No concerns on my end. I just didn't know if I missed something and wasn't going to push the change too hard.

philipp-spiess

This looks fine for me as well 🙂

Instead of focusing on your follow ups we should probably start to lay out the „new“ event system Dan mentioned in #13613 (comment) - Ideally all of those issues will be irrelevant by then.

philipp-spiess · 2018-09-11T16:38:52Z

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

@@ -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;


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

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,

);

}

};

}

gaearon · 2018-09-11T16:50:28Z

packages/react-dom/src/events/ReactDOMEventListener.js

@@ -57,6 +57,8 @@ function getTopLevelCallbackBookKeeping(
  targetInst: Fiber | null,
  ancestors: Array<Fiber>,
 } {
+  const topLevelType = unsafeCastStringToDOMTopLevelType(nativeEvent.type);


What's the guideline for when it's "safe" to call it? How do we know it's okay here?

I think it's safe here because we know the nativeEvent type is always a DOMTopLevelType.

This is otherwise only used to cast the top level types themselves when they are created, like:

react/packages/react-dom/src/events/DOMTopLevelEventTypes.js

Lines 25 to 31 in f6fb03e

export const TOP_ABORT = unsafeCastStringToDOMTopLevelType('abort');

export const TOP_ANIMATION_END = unsafeCastStringToDOMTopLevelType(

getVendorPrefixedEventName('animationend'),

);

export const TOP_ANIMATION_ITERATION = unsafeCastStringToDOMTopLevelType(

getVendorPrefixedEventName('animationiteration'),

);

I wonder if there is an existing concept of a DOM event's type string. We don't really have top level types in anymore: just the browser standard event names.

Should have a comment explaining why then.

Commented! Part of me wonders if these could be annotated using the actual event type declarations here:

https://github.com/facebook/flow/blob/08f854876cc72165f1f7a2e023bd8e79e8a7fa02/lib/dom.js#L144-L153

If that were the case, we could remove the concept of a top level type and just make it an DOM event type

gaearon · 2018-09-11T19:06:10Z

packages/react-dom/src/events/ReactDOMEventListener.js

@@ -149,7 +152,7 @@ export function trapBubbledEvent(
    element,
    getRawEventName(topLevelType),
    // Check if interactive and wrap in interactiveUpdates


This comment should move above btw.

Done in 60de292

A previous change made it such that all top level event types correspond to their associated native event string values. This commit eliminates the .bind attached to dispatch and fixes a related flow type.

nhunzaker · 2018-09-12T18:41:51Z

@gaearon I think this is good to go, but would you like me to hold off on a merge during the sync?

gaearon · 2018-09-12T20:19:44Z

It's too late for sync today (spent more time than I thought) so I'll finish it tomorrow. And I plan to cut an open source patch on Monday. So let's wait until then.

gaearon · 2018-09-14T23:34:17Z

(For context: since @acdlite merged the Scheduler change we'd need to sync on Monday anyway. So this doesn't have to delay now.)

nhunzaker · 2018-09-14T23:38:56Z

Haha. Thanks. I wonder if anyone thought I was going rogue 😅

This reverts commit 0c9c591.

* Revert "Do not bind topLevelType to dispatch (#13618)" This reverts commit 0c9c591.

* Do not bind topLevelType to dispatch A previous change made it such that all top level event types correspond to their associated native event string values. This commit eliminates the .bind attached to dispatch and fixes a related flow type. * Add note about why casting event.type to a topLevelType is safe * Move interactiveUpdates comment to point of assignment

* Revert "Do not bind topLevelType to dispatch (facebook#13618)" This reverts commit 0c9c591.

* Do not bind topLevelType to dispatch A previous change made it such that all top level event types correspond to their associated native event string values. This commit eliminates the .bind attached to dispatch and fixes a related flow type. * Add note about why casting event.type to a topLevelType is safe * Move interactiveUpdates comment to point of assignment

* Revert "Do not bind topLevelType to dispatch (facebook#13618)" This reverts commit 0c9c591.

nhunzaker requested review from philipp-spiess and gaearon September 11, 2018 14:54

facebook-github-bot added the CLA Signed label Sep 11, 2018

nhunzaker commented Sep 11, 2018

View reviewed changes

nhunzaker force-pushed the no-bind-dispatch branch from a88ce01 to b319235 Compare September 11, 2018 15:00

nhunzaker force-pushed the no-bind-dispatch branch from b319235 to ab0b248 Compare September 11, 2018 15:15

philipp-spiess approved these changes Sep 11, 2018

View reviewed changes

gaearon reviewed Sep 11, 2018

View reviewed changes

nhunzaker added 3 commits September 11, 2018 14:52

Do not bind topLevelType to dispatch

7d2b2f1

A previous change made it such that all top level event types correspond to their associated native event string values. This commit eliminates the .bind attached to dispatch and fixes a related flow type.

Add note about why casting event.type to a topLevelType is safe

4e1f1a6

Move interactiveUpdates comment to point of assignment

2581987

nhunzaker force-pushed the no-bind-dispatch branch from 60de292 to 2581987 Compare September 11, 2018 21:59

Akshat-Jain approved these changes Sep 14, 2018

View reviewed changes

nhunzaker merged commit 0c9c591 into facebook:master Sep 14, 2018

gaearon added a commit that referenced this pull request Sep 17, 2018

Revert "Do not bind topLevelType to dispatch (#13618)"

22cd5a9

This reverts commit 0c9c591.

gaearon mentioned this pull request Sep 17, 2018

Revert "Do not bind topLevelType to dispatch" #13674

Merged

gaearon added a commit that referenced this pull request Sep 17, 2018

Revert "Do not bind topLevelType to dispatch" (#13674)

a210b5b

* Revert "Do not bind topLevelType to dispatch (#13618)" This reverts commit 0c9c591.

Simek pushed a commit to Simek/react that referenced this pull request Oct 25, 2018

Revert "Do not bind topLevelType to dispatch" (facebook#13674)

8cffe0e

* Revert "Do not bind topLevelType to dispatch (facebook#13618)" This reverts commit 0c9c591.

jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019

Revert "Do not bind topLevelType to dispatch" (facebook#13674)

a5f11b0

* Revert "Do not bind topLevelType to dispatch (facebook#13618)" This reverts commit 0c9c591.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not bind topLevelType to dispatch #13618

Do not bind topLevelType to dispatch #13618

nhunzaker commented Sep 11, 2018 •

edited

nhunzaker Sep 11, 2018 •

edited

nhunzaker Sep 11, 2018

gaearon Sep 11, 2018

nhunzaker Sep 11, 2018

pull-bot commented Sep 11, 2018 •

edited

schedule

gaearon commented Sep 11, 2018

nhunzaker commented Sep 11, 2018

philipp-spiess left a comment

philipp-spiess Sep 11, 2018

nhunzaker Sep 11, 2018 •

edited

gaearon Sep 11, 2018

nhunzaker Sep 11, 2018

gaearon Sep 11, 2018

nhunzaker Sep 11, 2018

gaearon Sep 11, 2018

nhunzaker Sep 11, 2018

nhunzaker commented Sep 12, 2018

gaearon commented Sep 12, 2018

gaearon commented Sep 14, 2018

nhunzaker commented Sep 14, 2018

	function getListeningForDocument(mountAt: any) {
	// In IE8, `mountAt` is a host object and doesn't have `hasOwnProperty`
	// directly.
	if (!Object.prototype.hasOwnProperty.call(mountAt, topListenersIDKey)) {
	mountAt[topListenersIDKey] = reactTopListenersCounter++;
	alreadyListeningTo[mountAt[topListenersIDKey]] = {};
	}
	return alreadyListeningTo[mountAt[topListenersIDKey]];
	}

	const isListening = getListeningForDocument(mountAt);
	const dependencies = registrationNameDependencies[registrationName];

	for (let i = 0; i < dependencies.length; i++) {
	const dependency = dependencies[i];
	if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) {

	// Track whether all listeners exists for this plugin. If none exist, we do
	// not extract events. See #3639.
	if (!doc \|\| !isListeningToAllDependencies('onSelect', doc)) {
	return null;
	}

	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,
	);
	}
	};
	}

	export const TOP_ABORT = unsafeCastStringToDOMTopLevelType('abort');
	export const TOP_ANIMATION_END = unsafeCastStringToDOMTopLevelType(
	getVendorPrefixedEventName('animationend'),
	);
	export const TOP_ANIMATION_ITERATION = unsafeCastStringToDOMTopLevelType(
	getVendorPrefixedEventName('animationiteration'),
	);

Do not bind topLevelType to dispatch #13618

Do not bind topLevelType to dispatch #13618

Conversation

nhunzaker commented Sep 11, 2018 • edited

Why

Why drop bind()

Could it be faster?

Challenges

nhunzaker Sep 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented Sep 11, 2018 • edited

schedule

gaearon commented Sep 11, 2018

nhunzaker commented Sep 11, 2018

philipp-spiess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Sep 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Sep 12, 2018

gaearon commented Sep 12, 2018

gaearon commented Sep 14, 2018

nhunzaker commented Sep 14, 2018

nhunzaker commented Sep 11, 2018 •

edited

nhunzaker Sep 11, 2018 •

edited

pull-bot commented Sep 11, 2018 •

edited

nhunzaker Sep 11, 2018 •

edited