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

Expose less internals for TestUtils #13539

Merged
merged 3 commits into from
Sep 3, 2018
Merged

Expose less internals for TestUtils #13539

merged 3 commits into from
Sep 3, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 2, 2018

This makes internal exports more targeted. Using an array should minify better because we don't create intermediate objects for modules, and can crush function names. These internals should only be used by TestUtils and UnstableNativeDependencies, I've updated both.

  • TODO before merging: we need to check www for which of these are being used, and then move them to ReactDOMFB if necessary.

@@ -739,14 +739,19 @@ const ReactDOM: Object = {
unstable_flushControlled: DOMRenderer.flushControlled,

__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
// For TapEventPlugin which is popular in open source
EventPluginHub,
Copy link
Collaborator Author

@gaearon gaearon Sep 2, 2018

Choose a reason for hiding this comment

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

Technically this was used by TapEventPlugin. But it's already broken with 16.4 and has been deprecated: https://github.com/zilverline/react-tap-event-plugin#deprecated. So this doesn't matter in practice.

I'll check what's up with our www version but last time I looked at it, there were 5 or so callsites left.

dispatchEvent,
runEventsInBatch,
// eslint-disable-next-line no-unused-vars
getFiberCurrentPropsFromNode,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to make this just , but Prettier kept moving my comment so I left it in.

@@ -450,19 +453,6 @@ function buildSimulators() {
}
}

// Rebuild ReactTestUtils.Simulate whenever event plugins are injected
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't need this because we removed injection.

ReactDOMEventListener,
// Keep in sync with ReactDOMUnstableNativeDependencies.js
// and ReactTestUtils.js:
Events: [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could've made the whole thing an array, but our FB bundle puts some additional exports here. We'll need to come back to them and clean them up too.

@pull-bot
Copy link

pull-bot commented Sep 2, 2018

ReactDOM: size: -1.3%, gzip: -1.0%

Details of bundled changes.

Comparing: 28b9289...a691a94

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js -0.3% -0.3% 646.54 KB 644.34 KB 151.28 KB 150.89 KB UMD_DEV
react-dom.production.min.js -1.3% -1.0% 93.32 KB 92.14 KB 30.37 KB 30.07 KB UMD_PROD
react-dom.development.js -0.3% -0.3% 641.85 KB 639.64 KB 149.91 KB 149.51 KB NODE_DEV
react-dom.production.min.js -1.3% -1.5% 93.28 KB 92.08 KB 30.13 KB 29.66 KB NODE_PROD
react-dom-test-utils.development.js -1.0% -0.4% 45.07 KB 44.6 KB 12.23 KB 12.18 KB UMD_DEV
react-dom-test-utils.production.min.js -4.9% -4.0% 10.5 KB 9.99 KB 3.87 KB 3.71 KB UMD_PROD
react-dom-test-utils.development.js -1.0% -0.4% 44.79 KB 44.32 KB 12.16 KB 12.12 KB NODE_DEV
react-dom-test-utils.production.min.js -5.2% -3.9% 10.3 KB 9.76 KB 3.8 KB 3.65 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js +0.3% +0.4% 60.26 KB 60.42 KB 15.76 KB 15.83 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js -0.7% 🔺+0.1% 11.15 KB 11.08 KB 3.83 KB 3.83 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js +0.3% +0.4% 59.93 KB 60.09 KB 15.63 KB 15.7 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js -0.7% -0.1% 10.9 KB 10.83 KB 3.74 KB 3.74 KB NODE_PROD
ReactDOM-dev.js -0.2% -0.1% 663.63 KB 661.97 KB 151.92 KB 151.84 KB FB_WWW_DEV
ReactDOM-prod.js -0.3% -0.3% 287.71 KB 286.89 KB 53.3 KB 53.15 KB FB_WWW_PROD
ReactTestUtils-dev.js -1.1% -0.4% 41.88 KB 41.41 KB 11.21 KB 11.16 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-dev.js +0.3% +0.5% 57.63 KB 57.78 KB 14.58 KB 14.64 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-prod.js -0.2% 🔺+0.1% 26.59 KB 26.53 KB 5.32 KB 5.33 KB FB_WWW_PROD
react-dom.profiling.min.js -1.2% -1.4% 96.14 KB 94.94 KB 30.75 KB 30.33 KB NODE_PROFILING
ReactDOM-profiling.js -0.3% -0.3% 294.13 KB 293.31 KB 54.68 KB 54.52 KB FB_WWW_PROFILING

react-scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-scheduler.development.js +24.4% +23.6% 15.4 KB 19.17 KB 4.65 KB 5.74 KB UMD_DEV
react-scheduler.production.min.js 🔺+15.8% 🔺+21.0% 2.73 KB 3.16 KB 1.26 KB 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

ReactDOMComponentTree,
ReactDOMEventListener,
// Keep in sync with ReactDOMUnstableNativeDependencies.js
// and ReactTestUtils.js:
Copy link
Contributor

@bvaughn bvaughn Sep 3, 2018

Choose a reason for hiding this comment

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

I think we could use Flow to type the Array below to keep these two files in sync for us, e.g.:

Events: [Foo, Bar, Baz] = [ ... ]

Like so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how many of these methods are typed but we can try.

Copy link
Collaborator Author

@gaearon gaearon Sep 3, 2018

Choose a reason for hiding this comment

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

That still wouldn’t help if two methods have the same signature but you mess up their order. So I’m not sure it’s that helpful. And if you need to be careful anyway then maybe it’s okay to keep as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, it wouldn't be perfect, but even if all the types were the same– it would warn if we added/removed an arg one place and not the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed, I tried this but some of those are untyped, and it seemed like more work than it's worth

@necolas
Copy link
Contributor

necolas commented Sep 7, 2018

Hi. Someone reported that this broke React Native for Web, which relies on EventPluginHub.injection.injectEventPluginsByName.

necolas/react-native-web#1096

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 7, 2018

Hmm. I thought that everything RNW depends on hangs off the ReactDOMUnstableNativeDependencies entry point.

I’m happy to add more stuff to that entry point, but RNW shouldn’t read anything from main secret exports.

@necolas
Copy link
Contributor

necolas commented Sep 7, 2018

Sure, I don't mind where it's made available. Last time there were changes this is where y'all put it. If you can add it to ReactDOMUnstableNativeDependencies and put that out in a patch, that would be great

@necolas
Copy link
Contributor

necolas commented Sep 7, 2018

Created a new issue to track this bug #13589

aweary pushed a commit to aweary/react that referenced this pull request Sep 8, 2018
EventPluginHub

injectComponentTree was exposed for react-native-web, but wasn't
actually being used by the project. They were using EventPluginHub
through ReactDOM's secret internals, but that was removed in facebook#13539

This removes the unused injectComponentTree export, refactors the
ResponderEventPlugin test so it doesn't depend on it, and also adds
EventPluginHub to the exports to unbreak react-native-web
aweary added a commit that referenced this pull request Sep 8, 2018
…ntPluginHub (#13598)

* Remove injectComponentTree from unstable-native-dependencies, add
EventPluginHub

injectComponentTree was exposed for react-native-web, but wasn't
actually being used by the project. They were using EventPluginHub
through ReactDOM's secret internals, but that was removed in #13539

This removes the unused injectComponentTree export, refactors the
ResponderEventPlugin test so it doesn't depend on it, and also adds
EventPluginHub to the exports to unbreak react-native-web

* Re-export injectEventPluginsByName from ReactDOM internals
Simek pushed a commit to Simek/react that referenced this pull request Oct 25, 2018
…ntPluginHub (facebook#13598)

* Remove injectComponentTree from unstable-native-dependencies, add
EventPluginHub

injectComponentTree was exposed for react-native-web, but wasn't
actually being used by the project. They were using EventPluginHub
through ReactDOM's secret internals, but that was removed in facebook#13539

This removes the unused injectComponentTree export, refactors the
ResponderEventPlugin test so it doesn't depend on it, and also adds
EventPluginHub to the exports to unbreak react-native-web

* Re-export injectEventPluginsByName from ReactDOM internals
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
* Expose less internals for TestUtils

* Keep EventPluginHub for www

* Reorder to simplify
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…ntPluginHub (facebook#13598)

* Remove injectComponentTree from unstable-native-dependencies, add
EventPluginHub

injectComponentTree was exposed for react-native-web, but wasn't
actually being used by the project. They were using EventPluginHub
through ReactDOM's secret internals, but that was removed in facebook#13539

This removes the unused injectComponentTree export, refactors the
ResponderEventPlugin test so it doesn't depend on it, and also adds
EventPluginHub to the exports to unbreak react-native-web

* Re-export injectEventPluginsByName from ReactDOM internals
flyskywhy added a commit to flyskywhy/react-web that referenced this pull request Sep 5, 2019
… react-native@0.59.10 for [Expose less internals for TestUtils](facebook/react#13539)
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.

5 participants