ReactDOM.useEvent: Add more scaffolding for useEvent hook #18271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trueadm
requested review from
threepointone,
bvaughn,
sebmarkbage,
necolas and
gaearon
facebook-github-bot
added
React Core Team
|
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 8244805:
|
Details of bundled changes.Comparing: a3bf668...8244805 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: a3bf668...8244805 react-native-renderer
react-art
react-test-renderer
react-dom
react-reconciler
ReactDOM: size: 🔺+0.3%, gzip: 🔺+0.4% Size changes (stable) |
bvaughn
reviewed
Mar 10, 2020
packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js
Outdated
Show resolved
Hide resolved
trueadm
force-pushed
the
use-event-2
branch
2 times, most recently
from
e14e5c0
to
7f2ca55
Compare
bvaughn
reviewed
Mar 10, 2020
trueadm
force-pushed
the
use-event-2
branch
3 times, most recently
from
6bd450e
to
72d8aba
Compare
|
@trueadm you have added react-dom-16.13.0.tgz. it's a mistake? |
|
@ecreeth Nicly spotted. Thanks :) |
trueadm
force-pushed
the
use-event-2
branch
3 times, most recently
from
f438e45
to
f65c46c
Compare
trueadm
force-pushed
the
use-event-2
branch
3 times, most recently
from
bd161c8
to
87ed733
Compare
bvaughn
approved these changes
Mar 10, 2020
| @@ -26,6 +26,10 @@ import ReactNativeFiberHostComponent from './ReactNativeFiberHostComponent'; | |||
|
|
|||
| const {get: getViewConfigForType} = ReactNativeViewConfigRegistry; | |||
|
|
|||
| export type ReactListenerEvent = Object; | |||
| export type ReactListenerMap = Object; | |||
| export type ReactListener = Object; | |||
There was a problem hiding this comment.
I wonder if you considered using a shared type between the reconciler and non-reconciler based renderers?
Do we expect that the type of ReactListenerMap and ReactListener will vary between renderers? I guess they would if ReactListenerEvent varied... maybe it would be hard to manage this with Flow generics.
I'm just a little concerned that Flow doesn't have our back here to keep all of the renderers in sync.
There was a problem hiding this comment.
I considered it, but I think that will largely become something to consider once we actually add these to those renderers. Given this is only exported (for now anyway) on ReactDOM, I don't see this as a blocker. In the long-term, we'd probably expose slightly different API. Like you mention, doing this with generics can become a bit of a pain, so I wanted to keep it as Object for now, just to make this work-load easier to get into the internal testing phase.
Add test Fix Address feedback fix Fix flow types Fix Fix Wtf? Fix path Fix Fix bundles... Fix CI tests, by adding to experimental build Attempt #2 at fixing CI Revert Fix Fix lint
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a follow up to #18267. Specifically, this adds a bit more of the
useEventhook implementation (but most just leaves most the code as TODOs). Furthermore, there's a very simple test that validates that the hook works in terms of returning the same event listener map.