Add flow to SyntheticEvent #19564
Add flow to SyntheticEvent #19564
Conversation
|
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 6d4d97f:
|
| @@ -923,7 +923,7 @@ function accumulateEnterLeaveListenersForEvent( | |||
| inCapturePhase: boolean, | |||
| ): void { | |||
| const registrationName = event._reactName; | |||
| if (registrationName === undefined) { | |||
| if (registrationName === null) { | |||
There was a problem hiding this comment.
Right now this can never be true:
accumulateEnterLeaveListenersForEventis called fromaccumulateEnterLeaveTwoPhaseListenersaccumulateEnterLeaveTwoPhaseListenersis called fromEnterLeaveEventPlugin#extractEventswith synthetic events that are created from nativemouseover,pointerover,mouseoutorpointerout- for these native events we have a statically defined
_reactNameinuserBlockingPairsForSimpleEventPlugin
It might make sense to add an invariant or just cast it to a string with a hint to the current data flow.
There was a problem hiding this comment.
I don't understand this change.
If it was previously sometimes undefined, then this changes behavior.
If it was previously sometimes null, then this changes behavior.
If it was neither, then this line can be removed, and the type can be made more strict.
Does that make sense?
There was a problem hiding this comment.
I think maybe you're saying that the type is more loose but for this particular case, we only get a subtype? Can we codify that in types?
There was a problem hiding this comment.
If it was previously sometimes undefined, then this changes behavior.
Not according to the flow types. Maybe there are still some files where SyntheticEvent is used that are uncovered?
If it was previously sometimes null, then this changes behavior.
It does change but there weren't any test so we're definitely missing some code coverage. This is what I meant with adding a test for reactName === null.
I think maybe you're saying that the type is more loose but for this particular case, we only get a subtype? Can we codify that in types?
Exactly. I'll try something like UnknownSyntheticEvent { reactName: null } and KnownSyntheticEvent { reactName: string } and see if we can get rid of some checks with earlier type checks.
There was a problem hiding this comment.
My impression is that this branch has something to do with the createEventHandle API. But I might be wrong. It would be good to look in git for why it was added.
There was a problem hiding this comment.
Can we codify that in types?
We can. One thing I noticed though is that new SyntheticEvent is not type safe at all. Flow seems to think that a generic object was created and doesn't actually type-check the properties at all: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgDKWAdhgBZ4YCWAxgKIBueZAKrgQLxgDeYATngCGtDADkhAWzwAuMAGcM-aiQDmYAL7ooAVxKjqcEkVIUqdJiwwAKAJS9UYMMGBgEBACZGA5BjBD5eWpVY0ERcSkCSkEAOjBHZ1cAAwpqeSSwLzx5MBI4P2pJHBg8aTIwJOIyShoGZjYODIAjHT9YRDdqGBgwEr8dHICgkLBqPww4CqqzWssG-AyhEiwEISwEwQwdfmNU+QBuVC1UF1zJvH5+OH4AGgqSPCRCOGkAMT0DIzsMnCumoSaMCwYAAVgM-JttiRBmBVCwLnQwHAmiC8KIwNh8P4SB5MnBsiRfP5RDohN1gbRKLQANYVPaLQLBEhlDDyVC0IyKMB6eRCKB4OYYOTTGoWeoYdhY7gPJ6mUV1Kx2Q7smBDEzVcwKsgAYVVgV4x1OFyu-EGOTwAA98KI8B52Zy-Lz+YLhXLNYLJVxco91TMxVZdUM7KggA
So right now any call to a function expecting ReactSyntheticEventType with new SyntheticEvent is not sound.
There was a problem hiding this comment.
It would be good to look in git for why it was added.
991c3b8#diff-6b784480f6f3297c42bc2e6e46e6f943L829-R818
event.dispatchConfig.registrationName was undefined | string while reactName is assumed to be string (even though it was nullable and still is.
eps1lon
force-pushed
the
test/null-react-name
branch
from
5bb80b8
to
5f8358c
Compare
eps1lon
force-pushed
the
test/null-react-name
branch
2 times, most recently
from
d1b61e6
to
e75f163
Compare
eps1lon
force-pushed
the
test/null-react-name
branch
from
e75f163
to
08dcda6
Compare
| enterEventType, | ||
| eventTypePrefix + 'enter', | ||
| to, | ||
| nativeEvent, | ||
| nativeEventTarget, | ||
| eventInterface, | ||
| ); | ||
| enter.target = toNode; | ||
| enter.relatedTarget = fromNode; | ||
| // flow thinks `enter` could be `null` at this point |
There was a problem hiding this comment.
We have to declare it as let enter: KnownReactSyntheticEvent | null because we later re-assign it to null conditionally:
react/packages/react-dom/src/events/plugins/EnterLeaveEventPlugin.js
Lines 167 to 169 in 08dcda6
There was a problem hiding this comment.
I'm not familiar with flow so there might be a better alternative. The current statements seems sound to me. TypeScript is fine with it, but flow is not
There was a problem hiding this comment.
You could have two variables. First one is not nullable and that’s where you initialise. Then you copy it to a second nullable one.
There was a problem hiding this comment.
Took the liberty and flipped the assignment around. Instead of conditionally nulling we conditionally create the event. This means no more wasted work in the constructor.
Otherwise we'll end up with
let enter: SyntheticEvent | null = null;
const enterEvent: SyntheticEvent = { reactName: 'mouseleave' }
enterEvent.reactName = '';
enter = enterEventwhich looks odd. Or do you prefer that?
| // If we are not processing the first ancestor, then we | ||
| // should not process the same nativeEvent again, as we | ||
| // If we are processing the first ancestor, then we | ||
| // should process the same nativeEvent again, as we |
There was a problem hiding this comment.
This wording change makes the comment nonsensical. I'll reword.
|
Thanks! |
* Add flow to SyntheticEvent * Minimal implementation of known and unknown synthetic events * less casting * Update EnterLeaveEventPlugin.js Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
Summary
Type-check
SyntheticEventand unsound usage.According to the types there were a lot of unnecessary guards against
event._reactNamebeingnull. Removing all these checks does not fail any tests but produces unsound types. The origin of_reactNamebeingnullisreact/packages/react-dom/src/events/DOMPluginEventSystem.js
Line 1037 in 5bb80b8
Test Plan
_reactName === null?