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

Refactor DOM plugin system to fewer modules #18025

Merged

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Feb 12, 2020

This PR refactors and cleans up the existing logic around React's DOM event plugin system. This is primiary to make the system somewhat easier to fork and manage as part of the future work and investigation into possibly using React Roots as the delegation nodes rather than using only the document. Please note, there should be no logic/functionaly changes from this PR, and all existing code paths should still work.

Notably, this PR does the following:

  • We have a new module called DOMEventPluginSystem, this is where the bulk of the DOM plugin event system sits. This file should be forkable for future work with relative ease.
  • EventPluginHub has been removed. Instead the core functions have now moved to their respective renderers instead. This should make it easier to make changes to the DOM event system in the future without impacting other renderers that keep the existing plugin system.
  • getListener has been added. This function was previously in EventPluginHub, but now is a single module and exported function, as it is required during the event propagation phase.
  • ReactDOMEventListener previously had a lot of logic relating to the DOM plugin system. This has now been moved into DOMEventPluginSystem.
  • We had a test called EventPluginHub-test that was more a test for invalid listeners, so the test has changed name to reflect this better.
  • We directly inject plugins into the EventPluginRegistry rather than the EventPluginHub.
  • The DOM event plugin order has been inlined into ReactDOMClientInjection. In future React versions, we might want to remove this module and instead inline the event plugins directly into the event system instead to save bytes and improve runtime performance (RNW currently requires this as of React 16).
  • We should get a small size perf win for ReactDOM because we've removed some of the module indirection from before.

@trueadm trueadm changed the title Refactor DOM plugin system to single module Refactor DOM plugin system to fewer modules Feb 12, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 12, 2020

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 98f306c:

Sandbox Source
crazy-ramanujan-evjiy Configuration

@trueadm trueadm force-pushed the move-dom-plugin-logic-to-own-module branch from 86d7203 to 0de3d8f Compare February 12, 2020 12:53
return null;
}
listener = props[registrationName];
if (shouldPreventMouseEvent(registrationName, inst.type, props)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a shame that this runs for Fabric and RN too. This is definitely something we should remove once we get to clean up the existing DOM event system.

@@ -282,7 +282,7 @@ to return true:wantsResponderID| |
+ + */

/**
* A note about event ordering in the `EventPluginHub`.
* A note about event ordering in the `EventPluginRegistry`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We inject into the EventPluginRegistry now, so the comment changes are to reflect that.

@sizebot

This comment has been minimized.

@trueadm trueadm force-pushed the move-dom-plugin-logic-to-own-module branch from 0de3d8f to 98f306c Compare February 12, 2020 13:00
@sizebot
Copy link

sizebot commented Feb 12, 2020

Details of bundled changes.

Comparing: a607ea4...98f306c

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactFabric-prod.js 0.0% -0.0% 267.07 KB 267.2 KB 45.74 KB 45.73 KB RN_OSS_PROD
ReactNativeRenderer-dev.js -0.2% -0.2% 753.13 KB 751.72 KB 158.31 KB 158.03 KB RN_OSS_DEV
ReactFabric-profiling.js 0.0% -0.0% 278.25 KB 278.38 KB 47.88 KB 47.86 KB RN_OSS_PROFILING
ReactFabric-dev.js -0.2% -0.1% 743.74 KB 742.34 KB 156.1 KB 155.9 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 0.0% 🔺+0.1% 274.76 KB 274.89 KB 47.03 KB 47.08 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js 0.0% +0.1% 285.98 KB 286.11 KB 49.21 KB 49.26 KB RN_OSS_PROFILING

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom-test-utils.development.js 0.0% 0.0% 53.65 KB 53.65 KB 15.27 KB 15.27 KB NODE_DEV
react-dom.production.min.js -0.1% -0.2% 116.08 KB 115.96 KB 37.27 KB 37.21 KB UMD_PROD
react-dom-testing.development.js -0.1% -0.2% 960.41 KB 958.97 KB 215.03 KB 214.59 KB NODE_DEV
react-dom.profiling.min.js -0.1% -0.2% 119.61 KB 119.49 KB 38.45 KB 38.38 KB UMD_PROFILING
react-dom-testing.production.min.js -0.1% -0.2% 117.22 KB 117.08 KB 37.09 KB 37.03 KB NODE_PROD
react-dom.development.js -0.1% -0.2% 962.99 KB 961.55 KB 216.02 KB 215.57 KB NODE_DEV
react-dom-testing.profiling.min.js -0.1% -0.2% 120.9 KB 120.75 KB 38.2 KB 38.15 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.production.min.js -0.1% -0.2% 116.15 KB 116 KB 36.61 KB 36.55 KB NODE_PROD
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.3% 1.2 KB 1.2 KB 701 B 703 B UMD_PROD
react-dom.profiling.min.js -0.1% -0.2% 119.82 KB 119.67 KB 37.74 KB 37.67 KB NODE_PROFILING
react-dom-server.browser.development.js -0.1% -0.1% 139.3 KB 139.16 KB 36.94 KB 36.91 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 20 KB 20 KB 7.4 KB 7.4 KB UMD_PROD
react-dom-unstable-fizz.browser.development.js 0.0% +0.1% 3.7 KB 3.7 KB 1.49 KB 1.5 KB NODE_DEV
react-dom-unstable-fizz.browser.production.min.js 0.0% 🔺+0.2% 1.04 KB 1.04 KB 633 B 634 B NODE_PROD
react-dom-server.browser.development.js -0.1% -0.1% 135.23 KB 135.09 KB 35.91 KB 35.88 KB NODE_DEV
react-dom-test-utils.development.js 0.0% 0.0% 55.38 KB 55.38 KB 15.59 KB 15.59 KB UMD_DEV
react-dom-server.browser.production.min.js 0.0% -0.0% 19.93 KB 19.93 KB 7.39 KB 7.39 KB NODE_PROD
react-dom-test-utils.production.min.js 0.0% 0.0% 11.2 KB 11.2 KB 4.16 KB 4.16 KB UMD_PROD
react-dom-test-utils.production.min.js 0.0% -0.0% 10.97 KB 10.97 KB 4.09 KB 4.09 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 -3.6% -4.5% 60.92 KB 58.72 KB 16.07 KB 15.35 KB UMD_DEV
react-dom-unstable-fizz.node.production.min.js 0.0% 🔺+0.1% 1.2 KB 1.2 KB 688 B 689 B NODE_PROD
react-dom-unstable-native-dependencies.production.min.js 0.0% 🔺+0.2% 10.24 KB 10.24 KB 3.46 KB 3.47 KB UMD_PROD
react-dom-server.node.development.js -0.1% -0.1% 136.34 KB 136.2 KB 36.14 KB 36.11 KB NODE_DEV
react-dom-testing.development.js -0.1% -0.2% 966.33 KB 964.89 KB 216.72 KB 216.23 KB UMD_DEV
react-dom-server.node.production.min.js 0.0% -0.0% 20.34 KB 20.34 KB 7.54 KB 7.54 KB NODE_PROD
react-dom-testing.production.min.js -0.1% -0.1% 117.12 KB 117 KB 37.75 KB 37.71 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js -3.6% -4.5% 60.62 KB 58.42 KB 15.99 KB 15.28 KB NODE_DEV
react-dom.development.js -0.1% -0.2% 968.91 KB 967.47 KB 217.69 KB 217.2 KB UMD_DEV
react-dom-testing.profiling.min.js -0.1% -0.2% 120.64 KB 120.52 KB 38.91 KB 38.85 KB UMD_PROFILING
react-dom-unstable-native-dependencies.production.min.js -0.0% 🔺+0.1% 9.98 KB 9.98 KB 3.37 KB 3.37 KB NODE_PROD

ReactDOM: size: -0.1%, gzip: -0.2%

Size changes (stable)

Generated by 🚫 dangerJS against 98f306c

@trueadm trueadm requested a review from necolas February 12, 2020 21:52
Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

lgtm

@trueadm trueadm merged commit 9def56e into facebook:master Feb 14, 2020
@trueadm trueadm deleted the move-dom-plugin-logic-to-own-module branch February 14, 2020 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants