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.

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

[react-interactions] Add Listener API + useEvent hook #17651

Open
wants to merge 5 commits into
base: master
from

Conversation

@trueadm
Copy link
Member

trueadm commented Dec 18, 2019

Note: This API is intentionally meant to be a low-level way of creating events and assigning listeners to them. It's meant to be verbose so larger building blocks can be created on top of them.

This PR is an alternative solution and system to that of my other PR: #17508. Specifically, based off feedback internally, I've tried to tackle some of the problems that were brought up with the createListener approach in #17508:

  • createListener largely depended on events being registered in the commit phase, meaning that there would likely be issues around needing to flush more to ensure we register DOM events. The new approach enforces all events are registered unconditionally via hooks in the render phase, mitigating this issue.
  • createListener allowed for listeners to update and change their properties between renders, which again is problematic for performance and would also require more flushes to ensure we have committed the latest version of each listener.
  • createListener had a complex diffing process to ensure we stored the latest listeners, but this meant that the process had additional overhead and memory usage – which is no longer the case with this PR.
  • createListener required listeners to be put on nodes via the listeners prop. Furthermore, it required using arrays to combine multiple listeners, which some felt was not idealistic and might be confusing during debugging as to which listeners occurred at which stages. Also, there was general dislike to introducing another internal prop – as it would mean we'd have to first forbid listeners and wait for React 17 to introduce these APIs, as they might be used in the wild for other reasons (custom elements).
  • createListener didn't provide an idiomatic way to conditionally add/remove root events (they're called delegated events in this new PR). With the new approach, there's a streamlined approach to ensure this is easier to do, and by default, no root events can be added unconditionally, which is a code-smell and a good cause of memory leaks/performance issues.

Taking the above points into consideration, the design of this new event system aims at bringing the same capabilities as described in #17508 whilst also providing some other nice features, that should allow for bigger event sub-systems to be built on top.

ReactDOM.useEvent

This hook allows for the registration to a native DOM event, similar to that of addEventListener on the web. useEvent takes a given event type and registers it to the DOM then returns an object unique to that event that allows listeners to be attached to their targets in an effect or another event handler. The object provides three different methods to setup and handle listeners:

  • setListener(target: window | element, listener: ?(Event => void)) set a listener to be active for a given DOM node. The node must be a DOM node managed by React or it can be the window node for delegation purposes. If the listener is null or undefined then we remove the given listener for that DOM node or window object.
  • clear() remove all listeners

The hook takes three arguments:

  • type the DOM event to listen to
  • options an optional object allowing for additional properties to be defined on the event listener. The options are:
    • passive provide an optional flag that tells the listener to register a passive DOM event listener or an active DOM event listener
    • capture provide an optional flag that tells the listener callback to fire in the capture phase or the bubble phase
    • priority provide an optional Scheduler priority that allows React to correct schedule the listener callback to fire at the correct priority.

Note

For propagation, the same rules of stopPropagation and stopImmediatePropagation apply to these event listeners. These methods are actually monkey-patched, as we use the actual native DOM events with this system and API, rather than Synthetic Events. currentTarget and eventPhase are also respectfully monkey-patched to co-ordinate and align with the propagation system involved internally within React.

Furthermore, all event listeners are passive by default. If is desired to called event.preventDefault on an event listener, then the event listener should be made active via the passive option.

Examples

An example of a basic clickable button:

import {useRef, useEffect} from 'react';
import {useEvent} from 'react-dom';

function Button({children, onClick}) {
  const buttonRef = useRef(null);
  const clickEvent = useEvent('click');

  useEffect(() => {
    clickEvent.setListener(buttonRef.current, onClick);
  });

  return <button ref={buttonRef}>{children}</button>;
}

If you want to listen to events that are delegated to the window, you can do that:

import {useRef, useEffect} from 'react';
import {useEvent} from 'react-dom';

function Button({children, onClick}) {
  const clickEvent = useEvent('click');

  useEffect(() => {
    // Pass in `window`, which is supported with this API
    // Note: `window` is not supported, as React doesn't
    // listen to events that high up.
    clickEvent.setListener(window, onClick);
  });

  return <button>{children}</button>;
}

If you wanted to extract the verbosity out of this into a custom hook, then it's possible to do so:

import {useRef, useEffect} from 'react';
import {useEvent} from 'react-dom';

function useClick(ref, onClick) {
  const clickEvent = useEvent('click');

  useEffect(() => {
    clickEvent.setListener(ref.current, onClick);
  });

  return buttonRef;
}

function Button({children}) {
  const buttonRef = useRef(null);
  useClick(buttonRef , () => {
    console.log('Hello world!')
  });
  return <button ref={buttonRef}>{children}</button>;
}

A more complex button that tracks when the button is being pressed with the mouse:

import {useRef, useEffect} from 'react';
import {useEvent} from 'react-dom';

function Button({children, onClick}) {
  const buttonRef = useRef(null);
  const [pressed, setPressed] = useState(false);
  const click = useEvent('click');
  const mouseUp = useEvent('mouseup');
  const mouseDown = useEvent('mousedown');

  useEffect(() => {
    const button = buttonRef.current;

    const handleMouseUp = () => {
      setPressed(false);
    };
    const handleMouseDown = () => {
      setPressed(true); 
    };

    click.setListener(button, onClick);
    if (pressed) {
      mouseUp.setListener(button, handleMouseUp);
    } else {
      mouseDown.setListener(button, handleMouseDown);
    }

    return () => {
      click.setListener(button, null);
      mouseDown.setListener(button, null);
      mouseUp.setListener(button, null);
    };
  }, [pressed, onClick]);

  return (
    <button ref={buttonRef} className={pressed && 'pressed'}>
      {children}
    </button>
  );
}

What about the DOM's element.addEventListener?

In many respects, this low-level API was intentionally designed to be a replacement for the DOM's addEventListener. Not only does this new Listener API provide many nice benefits, like auto-recycling listeners on unmount, but it should also help prevent bugs that will likely occur when addEventListener is used in conjunction with Concurrent Mode.

For a detailed list of differences, here are just some of the key benefits of the Listener API vs the DOM's addEventListener:

  • The event listeners automatically get recycled upon unmount
  • The event listeners correctly batch updates and flush previous ones correctly, as well as co-ordinating priority levels with the internal scheduler
  • The event listeners allow for alignment with the propagation of other events in the React event system, something not possible with manual DOM event listeners
  • The event listeners will correctly work with React Portals, Suspense and Concurrent Mode, native event listeners do not.
  • By ensuring we use a hook, like useEvent, we can determine the event needed during server-side render and ensure those events are registered on initial hydration ahead of time. This makes it possible to "replay" those events before the components themselves have concurrently hydrated on the client.
@codesandbox

This comment has been minimized.

Copy link

codesandbox bot commented Dec 18, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7035587:

Sandbox Source
happy-http-b5um4 Configuration
@trueadm trueadm force-pushed the trueadm:responder-api branch from 23eaa87 to 7759719 Dec 18, 2019
@sizebot

This comment has been minimized.

Copy link

sizebot commented Dec 18, 2019

Details of bundled changes.

Comparing: b7f217d...7035587

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-dev.js +0.8% +0.7% 741.53 KB 747.34 KB 155.7 KB 156.8 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 266.8 KB 266.96 KB 45.77 KB 45.79 KB RN_OSS_PROD
ReactNativeRenderer-dev.js +0.8% +0.7% 750.91 KB 756.73 KB 157.91 KB 159 KB RN_OSS_DEV
ReactFabric-profiling.js +0.1% +0.1% 277.96 KB 278.12 KB 47.89 KB 47.92 KB RN_OSS_PROFILING
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.1% 274.37 KB 274.54 KB 47.02 KB 47.05 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.1% +0.1% 285.58 KB 285.75 KB 49.17 KB 49.2 KB RN_OSS_PROFILING

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 72.52 KB 72.59 KB 21.31 KB 21.33 KB NODE_PROD
react-reconciler.development.js +1.0% +0.9% 609.78 KB 615.72 KB 127.69 KB 128.81 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 72.51 KB 72.58 KB 21.31 KB 21.33 KB NODE_PROD
react-reconciler-reflection.development.js 0.0% 0.0% 19.51 KB 19.52 KB 6.39 KB 6.4 KB NODE_DEV
react-reconciler-reflection.production.min.js 0.0% 🔺+0.1% 2.85 KB 2.85 KB 1.24 KB 1.24 KB NODE_PROD
react-reconciler-persistent.development.js +1.0% +0.9% 606.97 KB 612.91 KB 126.5 KB 127.63 KB NODE_DEV

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-server.browser.production.min.js 🔺+0.2% 🔺+0.4% 19.94 KB 19.99 KB 7.4 KB 7.42 KB NODE_PROD
react-dom-test-utils.development.js 0.0% 0.0% 53.05 KB 53.06 KB 15.08 KB 15.08 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 115.95 KB 116.04 KB 37.3 KB 37.33 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.59 KB 60.59 KB 15.99 KB 15.99 KB NODE_DEV
react-dom.profiling.min.js +0.1% +0.1% 119.57 KB 119.66 KB 38.46 KB 38.49 KB UMD_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 9.96 KB 9.96 KB 3.36 KB 3.37 KB NODE_PROD
react-dom.development.js +2.2% +1.8% 953.5 KB 974.19 KB 213.83 KB 217.72 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 116.01 KB 116.1 KB 36.63 KB 36.65 KB NODE_PROD
react-dom.profiling.min.js +0.1% +0.1% 119.77 KB 119.86 KB 37.75 KB 37.78 KB NODE_PROFILING
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.87 KB 3.87 KB 1.54 KB 1.54 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.2 KB 1.2 KB 703 B 704 B UMD_PROD
react-dom-server.browser.development.js +0.1% +0.1% 138.88 KB 138.99 KB 36.78 KB 36.81 KB UMD_DEV
react-dom-server.browser.production.min.js 🔺+0.3% 🔺+0.4% 20.01 KB 20.06 KB 7.41 KB 7.44 KB UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.7 KB 3.7 KB 1.5 KB 1.5 KB NODE_DEV
react-dom-test-utils.development.js 0.0% 0.0% 54.78 KB 54.78 KB 15.4 KB 15.4 KB UMD_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.04 KB 1.04 KB 634 B 635 B NODE_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 11.17 KB 11.17 KB 4.14 KB 4.14 KB UMD_PROD
react-dom-server.browser.development.js +0.1% +0.1% 134.81 KB 134.92 KB 35.76 KB 35.78 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.94 KB 10.94 KB 4.08 KB 4.08 KB NODE_PROD
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 4.4 KB 4.4 KB 1.64 KB 1.64 KB NODE_DEV
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.89 KB 60.89 KB 16.06 KB 16.07 KB UMD_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.3% 1.2 KB 1.2 KB 689 B 691 B NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 10.22 KB 10.22 KB 3.47 KB 3.47 KB UMD_PROD
react-dom-server.node.development.js +0.1% +0.1% 135.92 KB 136.03 KB 35.99 KB 36.02 KB NODE_DEV
react-dom.development.js +2.2% +1.8% 959.43 KB 980.11 KB 215.51 KB 219.43 KB UMD_DEV
react-dom-server.node.production.min.js 🔺+0.2% 🔺+0.3% 20.34 KB 20.39 KB 7.55 KB 7.57 KB NODE_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 104.77 KB 104.84 KB 31.83 KB 31.85 KB UMD_PROD
react-art.development.js +0.8% +0.7% 678.09 KB 683.75 KB 145.94 KB 147 KB UMD_DEV
react-art.development.js +0.9% +0.8% 608.77 KB 614.43 KB 128.52 KB 129.6 KB NODE_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 69.77 KB 69.84 KB 21 KB 21.03 KB NODE_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer-shallow.development.js +0.4% +0.3% 37.88 KB 38.01 KB 9.83 KB 9.87 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+0.4% 🔺+0.9% 11.64 KB 11.69 KB 3.59 KB 3.62 KB UMD_PROD
react-test-renderer-shallow.development.js +0.4% +0.4% 32.42 KB 32.55 KB 8.52 KB 8.56 KB NODE_DEV
react-test-renderer-shallow.production.min.js 🔺+0.4% 🔺+0.5% 11.78 KB 11.83 KB 3.7 KB 3.72 KB NODE_PROD
react-test-renderer.development.js +0.9% +0.8% 622.86 KB 628.53 KB 131.47 KB 132.55 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.3% 71.91 KB 71.98 KB 21.92 KB 21.98 KB UMD_PROD
react-test-renderer.development.js +0.9% +0.8% 618.13 KB 623.8 KB 130.29 KB 131.37 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 71.61 KB 71.68 KB 21.54 KB 21.57 KB NODE_PROD

ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1%

Size changes (stable)

Generated by 🚫 dangerJS against 7035587

@sizebot

This comment has been minimized.

Copy link

sizebot commented Dec 18, 2019

Details of bundled changes.

Comparing: b7f217d...7035587

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.9% +0.8% 622.89 KB 628.56 KB 131.48 KB 132.56 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.2% 71.93 KB 72.01 KB 21.94 KB 21.99 KB UMD_PROD
react-test-renderer-shallow.development.js +0.4% +0.4% 37.89 KB 38.02 KB 9.84 KB 9.88 KB UMD_DEV
react-test-renderer-shallow.production.min.js 🔺+0.4% 🔺+0.9% 11.66 KB 11.71 KB 3.6 KB 3.63 KB UMD_PROD
react-test-renderer-shallow.development.js +0.4% +0.4% 32.43 KB 32.56 KB 8.53 KB 8.57 KB NODE_DEV
react-test-renderer-shallow.production.min.js 🔺+0.4% 🔺+0.6% 11.79 KB 11.84 KB 3.71 KB 3.73 KB NODE_PROD
react-test-renderer.development.js +0.9% +0.8% 618.15 KB 623.83 KB 130.3 KB 131.38 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% 🔺+0.1% 71.63 KB 71.71 KB 21.56 KB 21.59 KB NODE_PROD
ReactShallowRenderer-dev.js +0.4% +0.4% 34.43 KB 34.58 KB 8.49 KB 8.53 KB FB_WWW_DEV
ReactTestRenderer-dev.js +0.9% +0.8% 633.98 KB 639.8 KB 131.12 KB 132.22 KB FB_WWW_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.8% +0.7% 750.92 KB 756.74 KB 157.91 KB 159.01 KB RN_OSS_DEV
ReactFabric-dev.js +0.8% +0.7% 741.54 KB 747.36 KB 155.71 KB 156.81 KB RN_OSS_DEV
ReactNativeRenderer-dev.js +0.8% +0.7% 751.1 KB 756.91 KB 158.01 KB 159.1 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 266.81 KB 266.98 KB 45.77 KB 45.8 KB RN_OSS_PROD
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.1% 274.78 KB 274.94 KB 47.1 KB 47.13 KB RN_FB_PROD
ReactFabric-profiling.js +0.1% +0.1% 277.97 KB 278.14 KB 47.9 KB 47.93 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.1% 0.0% 285.98 KB 286.14 KB 49.25 KB 49.27 KB RN_FB_PROFILING
ReactNativeRenderer-prod.js 🔺+0.1% 🔺+0.1% 274.39 KB 274.55 KB 47.03 KB 47.05 KB RN_OSS_PROD
ReactFabric-dev.js +0.8% +0.7% 741.72 KB 747.54 KB 155.79 KB 156.89 KB RN_FB_DEV
ReactNativeRenderer-profiling.js +0.1% +0.1% 285.59 KB 285.76 KB 49.18 KB 49.2 KB RN_OSS_PROFILING
ReactFabric-prod.js 🔺+0.1% 🔺+0.1% 267.16 KB 267.33 KB 45.84 KB 45.87 KB RN_FB_PROD
ReactFabric-profiling.js +0.1% +0.1% 278.31 KB 278.48 KB 47.97 KB 47.99 KB RN_FB_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js +0.9% +0.9% 622.3 KB 628.1 KB 128.56 KB 129.66 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.5% 🔺+0.5% 236.29 KB 237.38 KB 39.8 KB 40.01 KB FB_WWW_PROD
react-art.development.js +0.8% +0.7% 678.11 KB 683.77 KB 145.94 KB 147.01 KB UMD_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 107.32 KB 107.4 KB 32.52 KB 32.55 KB UMD_PROD
react-art.development.js +0.9% +0.8% 608.79 KB 614.45 KB 128.52 KB 129.6 KB NODE_DEV
react-art.production.min.js 🔺+0.1% 🔺+0.1% 72.29 KB 72.36 KB 21.68 KB 21.7 KB NODE_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.profiling.min.js +0.1% +0.1% 123.81 KB 123.9 KB 38.8 KB 38.85 KB NODE_PROFILING
react-dom-server.browser.development.js +0.1% +0.1% 138.9 KB 139.01 KB 36.78 KB 36.81 KB UMD_DEV
react-dom-server.browser.production.min.js 🔺+0.3% 🔺+0.1% 20.47 KB 20.52 KB 7.5 KB 7.51 KB UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 54.79 KB 54.8 KB 15.4 KB 15.41 KB UMD_DEV
ReactDOMServer-prod.js 🔺+0.2% 🔺+0.2% 49 KB 49.08 KB 11.19 KB 11.21 KB FB_WWW_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.88 KB 3.88 KB 1.55 KB 1.55 KB UMD_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 11.18 KB 11.18 KB 4.15 KB 4.15 KB UMD_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.1% 1.21 KB 1.21 KB 711 B 712 B UMD_PROD
react-dom-test-utils.development.js 0.0% 0.0% 53.06 KB 53.07 KB 15.08 KB 15.08 KB NODE_DEV
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.71 KB 3.71 KB 1.5 KB 1.5 KB NODE_DEV
react-dom-test-utils.production.min.js 0.0% 0.0% 10.95 KB 10.95 KB 4.09 KB 4.09 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.05 KB 1.05 KB 642 B 643 B NODE_PROD
react-dom.development.js +2.2% +1.8% 959.46 KB 980.13 KB 215.53 KB 219.45 KB UMD_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 119.85 KB 119.94 KB 38.42 KB 38.45 KB UMD_PROD
react-dom.profiling.min.js +0.1% +0.1% 123.58 KB 123.67 KB 39.61 KB 39.64 KB UMD_PROFILING
react-dom.development.js +2.2% +1.8% 953.52 KB 974.21 KB 213.85 KB 217.74 KB NODE_DEV
react-dom-server.node.development.js +0.1% +0.1% 135.95 KB 136.06 KB 35.99 KB 36.02 KB NODE_DEV
react-dom.production.min.js 🔺+0.1% 🔺+0.1% 119.93 KB 120.02 KB 37.67 KB 37.71 KB NODE_PROD
react-dom-server.node.production.min.js 🔺+0.2% 🔺+0.3% 20.8 KB 20.85 KB 7.63 KB 7.65 KB NODE_PROD
react-dom-server.browser.development.js +0.1% +0.1% 134.84 KB 134.95 KB 35.76 KB 35.79 KB NODE_DEV
ReactDOM-dev.js +2.2% +1.9% 981.15 KB 1002.39 KB 216.71 KB 220.88 KB FB_WWW_DEV
react-dom-server.browser.production.min.js 🔺+0.2% 🔺+0.3% 20.39 KB 20.44 KB 7.47 KB 7.5 KB NODE_PROD
ReactDOM-prod.js 🔺+2.9% 🔺+3.1% 393.42 KB 405 KB 71.88 KB 74.08 KB FB_WWW_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.91 KB 60.91 KB 16.07 KB 16.07 KB UMD_DEV
ReactDOM-profiling.js +2.9% +2.9% 404.75 KB 416.32 KB 74.05 KB 76.23 KB FB_WWW_PROFILING
react-dom-unstable-native-dependencies.production.min.js 0.0% 0.0% 10.23 KB 10.23 KB 3.47 KB 3.48 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js 0.0% 0.0% 60.61 KB 60.61 KB 16 KB 16 KB NODE_DEV
react-dom-unstable-fizz.node.development.js 0.0% +0.1% 4.42 KB 4.42 KB 1.65 KB 1.65 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.1% 9.97 KB 9.97 KB 3.37 KB 3.38 KB NODE_PROD
ReactDOMServer-dev.js +0.1% +0.1% 140.24 KB 140.34 KB 35.49 KB 35.52 KB FB_WWW_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.3% 1.21 KB 1.21 KB 698 B 700 B NODE_PROD

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler-persistent.development.js +1.0% +0.9% 606.99 KB 612.93 KB 126.51 KB 127.64 KB NODE_DEV
react-reconciler-reflection.development.js 0.0% 0.0% 19.53 KB 19.53 KB 6.4 KB 6.4 KB NODE_DEV
react-reconciler-persistent.production.min.js 🔺+0.1% 🔺+0.1% 72.53 KB 72.6 KB 21.32 KB 21.34 KB NODE_PROD
react-reconciler-reflection.production.min.js 0.0% 🔺+0.2% 2.86 KB 2.86 KB 1.24 KB 1.25 KB NODE_PROD
react-reconciler.development.js +1.0% +0.9% 609.79 KB 615.74 KB 127.69 KB 128.81 KB NODE_DEV
react-reconciler.production.min.js 🔺+0.1% 🔺+0.1% 75.29 KB 75.36 KB 21.96 KB 21.98 KB NODE_PROD

ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.1%

Size changes (experimental)

Generated by 🚫 dangerJS against 7035587

@trueadm trueadm force-pushed the trueadm:responder-api branch 4 times, most recently from 16df9d7 to 72ed512 Dec 18, 2019
@necolas

This comment was marked as outdated.

Copy link
Contributor

necolas commented Dec 18, 2019

null or undefined can be used for the listener for cases when a listener is not desired.

Why?

Using stopImmediatePropgation will not do anything special, and instead will behave like stopPropgatation with a warning, as we feel that such a behavior is generally undesirable.

Please can you elaborate on why you consider it undesirable.

An additional method called event.skipPropagationForKind allows for event listeners of a given kind to be skipped.

I don't understand this feature. Please can you provide more details.

@trueadm trueadm force-pushed the trueadm:responder-api branch from 72ed512 to 54451f6 Dec 18, 2019
@trueadm

This comment was marked as outdated.

Copy link
Member Author

trueadm commented Dec 18, 2019

@necolas Hopefully these address your points:

Why?

It's desirable that listeners can be optional, for cases where you might reference props.onClick where the prop might be null or undefined. Furthermore, it might also be desirable that you listent to an event but don't want to action upon it (for example, a mouse event given you know you can use a pointer event). This can fast-path the reconcilation process and skip events that have nullish listener callbacks.

Please can you elaborate on why you consider it undesirable.

It's undesirable, although completely do-able because of how hooks work today. If you have many useEvent hooks on a function that listen to a given event, stopping propagation immediately will terminate them all, which means that many of the given effects in the same function cannot be fired. I'm on the fence with this once, as I'm happy to add it back but do we really need it?. I'm not sold on needing it, for the extra debugging complexity it adds when things go wrong (this includes the DOM today and the projects I've worked on in the past).

I don't understand this feature. Please can you provide more details.

This is a power feature, but one I added because the user-land alternative would be 2-3x more code. The logical scenario you might imagine is local propgation we had with React Flare. You can assign listeners to the same kind, such as a given unique Symbol. All listeners of that given Symbol then obey skipPropagationForKind when passed that exact symbol in terms of propagation order (capture, then bubble, as two different phases). This would allow user-land implementations of inclusive propagation to occur without needing lots of wiring in React components. I'm not too bothered about reverting this particular feature if we feel it's too elusive to the ideal of this PR though.

packages/shared/ReactFeatureFlags.js Outdated Show resolved Hide resolved
packages/shared/forks/ReactFeatureFlags.www.js Outdated Show resolved Hide resolved
@necolas

This comment was marked as outdated.

Copy link
Contributor

necolas commented Dec 19, 2019

it might also be desirable that you listen to an event but don't want to action upon it (for example, a mouse event given you know you can use a pointer event).

Why would you want to listen to an event but not do anything with it?

This can fast-path the reconcilation process and skip events that have nullish listener callbacks.

Is calling functions that don't do anything a significant issue? Doing that seems to be inherent to hooks.

I'm not sold on needing it, for the extra debugging complexity it adds when things go wrong (this includes the DOM today and the projects I've worked on in the past).

That's interesting because I think a lot of this is also true for stopPropagation. If you can stop an event propagating to other elements you might leave them in a broken state. For example, if I call stopPropagation on a mouseup but not a mousedown, an element higher up the tree might be stuck in a "press-down" state if the "end" event never reaches it.

This is a power feature, but one I added because the user-land alternative would be 2-3x more code…I'm not too bothered about reverting this particular feature if we feel it's too elusive to the ideal of this PR though.

Yeah I think we could start with a small footprint and see if there's anything we need to add as we start using it. In particular, I think modifying the event from the native event sets a bad precedent. Maybe we'd even be better off doing things like ReactDOM.someSpecialBehavior(event)

@trueadm

This comment was marked as outdated.

Copy link
Member Author

trueadm commented Dec 19, 2019

I can revert the skip logic, that’s fine. Do you feel I should re-add stopImmediatePropagation too? In terms of the nullish listener fallback, it’s something React supports today, I.e you can provide null to a given listener - if you feel strongly I can make it an invariant to do this - in that you must always provide a valid function as a fallback.

@necolas

This comment was marked as outdated.

Copy link
Contributor

necolas commented Dec 19, 2019

Do you feel I should re-add stopImmediatePropagation too?

For me it's a question of "what do we want this API to be?" If it is to be a low-level API over the native events, then I start wondering:

  • Why is the hook called useEvent and not useListener?
  • Why should we modify or remove fields from the native event?
  • Why can listeners "bind" to the function component? If that's useful in practical terms (is it?), perhaps it's something people should have to explicitly choose to do. At the moment it is the default pattern.
@trueadm trueadm force-pushed the trueadm:responder-api branch 2 times, most recently from 81c934b to 2f076e6 Dec 19, 2019
@trueadm

This comment was marked as outdated.

Copy link
Member Author

trueadm commented Dec 19, 2019

@necolas I've made those changes to the PR:

  • I've added stopImmediatePropagation
  • I've removed kind and the skipping of propagation

To address your further points:

Why is the hook called useEvent and not useListener?

I'm not sure it really matters, useEvent is kind of short for useEventListener, but then writing out useDelegatedEventListener is just terrible IMO. If we feel strongly that useListener is better than useEvent, then I'm down with that.

Why should we modify or remove fields from the native event?

We only monkey patch the native events that relate to propagation now. We no longer add any additional fields other than the internal field we need for tracking.

Why can listeners "bind" to the function component? If that's useful in practical terms (is it?), perhaps it's something people should have to explicitly choose to do. At the moment it is the default pattern.

I've gone back and forth on this one many times. I originally wanted to make bind mandatory, but then marking the function as a bind target is not as simple as it sounds – passing a reference to function is messy (what's stopping you from putting a reference to another closure?) as was marking it some kind of enum.

I think we need to look into the bind mechanism though, it's confusing and I think we can do better, so I agree with you there. Maybe we should setup a meeting to talk about it all.

@trueadm trueadm force-pushed the trueadm:responder-api branch from 2f076e6 to 0bec034 Dec 20, 2019
@trueadm trueadm changed the title [react-interactions] Add Listener API + useEvent/useDelegatedEvent [react-interactions] Add Listener API + useEvent hook Dec 20, 2019
@trueadm

This comment has been minimized.

Copy link
Member Author

trueadm commented Dec 20, 2019

I've redesign and refactored the code in this PR after much delibration and feedback today. Notably: this design is far more low-level and more like how events are added with addEventListener today. This is intentional and with this design, it actually makes it possible to be approaches like the createListener propsoal and PR with this design – plus it reduces the complexity and overhead needed when thinking about some of the problems to do with adding/removing events.

Here's another example of this looks like now:

function Component() {
  const divRef = useRef(null)
  // Creates a clickEvent that acts like a-kind of
  // Event Map. This should always happen in the
  // render phase, so we can properly setup the listeners
  // unconditionally. Furthermore, we can detect
  // this listeners during SSR, and eagerly setup
  // the right event listeners on init, to better
  // enhance event replaying (for custom events etc).
  const clickEvent = useEvent('click', {
    // You can leverage the same benefits
    // available today on the web, such as
    // passive event listeners.
    passive: true,
    capture: false,
  })

  useEffect(() => {
    const div = divRef.current;
    // Set a listener for a given DOM node
    clickEvent.setListener(div, nativeEvent => {
      console.log(nativeEvent);
    });
    // You can set a listener to the `window` or
    // any other node managed by React. Nodes
    // not managed by React will result in
    // an error being fired.
    clickEvent.setListener(window, nativeEvent => {
      console.log(nativeEvent);
    });
    // You can also delete a listener by passing `null`
    clickEvent.setListener(div, null);
    
    // If you have many listeners, you can clear them
    // all without having to know of them
    clickEvent.clear();

    // Note: listeners will automatically be
    // destroyed if the component with the hook
    // is detatched, or the DOM node that the
    // listener is attached to is detatched.
  }, [clickEvent])

  return <div ref={divRef} />
}
@trueadm trueadm requested a review from gaearon Dec 20, 2019
@trueadm trueadm force-pushed the trueadm:responder-api branch from 249c1bd to 2fee2fc Dec 21, 2019
@arianon

This comment has been minimized.

Copy link

arianon commented Dec 21, 2019

the useEvent API could be typed with type declarations such as:

in TS:

interface EventMap {
  "click": MouseEvent;
  "mouseup": MouseEvent;
  "focus": FocusEvent;
  "blur": FocusEvent;
  "keyup": KeyboardEvent;
  // ... and so on
}

function useEvent<K extends keyof EventMap>(type: K, options?: EventOptions): ReactDOMListenerMap<EventMap[K]>

and in Flow:

declare function useEvent(type: MouseEventTypes, options?: EventOptions): ReactDOMListenerMap<MouseEvent>;
declare function useEvent(type: KeyboardEventTypes, options?: EventOptions): ReactDOMListenerMap<KeyboardEvent>;
declare function useEvent(type: FocusEventTypes, options?: EventOptions): ReactDOMListenerMap<FocusEvent>;
// ... and so on
@bvaughn

This comment has been minimized.

Copy link
Contributor

bvaughn commented Dec 21, 2019

No prob Dominic!

That sounds like something that can easily be done in user-land too without us having to ship hundreds of these with ReactDOM etc

Strong agree. Big fan of solving this via generics or union types.

@trueadm trueadm force-pushed the trueadm:responder-api branch from 2fee2fc to 42881bc Dec 21, 2019
@trueadm

This comment has been minimized.

Copy link
Member Author

trueadm commented Dec 21, 2019

I've also updated the PR to now accept either window or a managed DOM element when using setListener() – it no longer supports document. Will need to do some more testing, but it seems to be fine. :)

@trueadm trueadm force-pushed the trueadm:responder-api branch 4 times, most recently from 0abbc84 to 85409f5 Dec 21, 2019
@necolas

This comment has been minimized.

Copy link
Contributor

necolas commented Dec 22, 2019

I don't think this API is sufficient for creating a global gesture system like the responder system in React Native. In that case you might only want to set up one listener for each touch event type on the window to track touches across the entire surface, and to coordinate how the interaction lock is managed across multiple views (accounting for views not managed by react).

@trueadm

This comment has been minimized.

Copy link
Member Author

trueadm commented Dec 22, 2019

@necolas How would this API be more suffecient for that use-case? Would you mind explaining what is missing?

I've also tweaked the API after @TrySound's suggestion to make setListener take null/undefined as the second argument to clear a listener rather than needing deleteListener. This improves code-size further dropping the entire implementation to under 1kb min+gzip! It also makes for less boilerplate for when you map a prop to a listener (which might be be nullish) which also helps reduce the size of userland custom hooks!

@trueadm trueadm force-pushed the trueadm:responder-api branch from 87c7c7d to c62180a Dec 22, 2019
@necolas

This comment has been minimized.

Copy link
Contributor

necolas commented Dec 23, 2019

I don't think there's anything wrong with this hooks API or that it necessarily needs (or can be) adjusted to support "global" systems like modality tracking and interaction-locks. They rely on a single coordinator that listens only to "top" level events on the window/document.

@devknoll

This comment has been minimized.

Copy link
Contributor

devknoll commented Jan 6, 2020

Out of curiosity: is there a chance that setting listeners in useEffect (rather than useLayoutEffect) could cause some events to be missed (however rare), or is there something else done to avoid that?

@trueadm

This comment has been minimized.

Copy link
Member Author

trueadm commented Jan 7, 2020

@devknoll You're right, in some cases you'd want to use useLayoutEffect, which works more like how events happen today in React. This is important for focus and load etc, but really, the most common cases can move into useEffect which means we do less work in the commit phase! :)

@trueadm trueadm force-pushed the trueadm:responder-api branch from c73efdb to 962feb1 Jan 28, 2020
Fix flow
@trueadm trueadm force-pushed the trueadm:responder-api branch from 962feb1 to 7035587 Jan 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.