[SelectInput] Fix event forwarding #8386
Conversation
|
I'm having problems with flow, which I'm not familiar with. |
src/Select/SelectInput.js
Outdated
| @@ -53,7 +53,7 @@ export type Props = { | |||
| * @param {object} event The event source of the callback | |||
| * @param {object} child The react element that was selected | |||
| */ | |||
| onChange?: (event: Object, child: Element<*>) => void, | |||
| onChange?: (event: SyntheticMouseEvent<*>, child: Element<*>) => void, | |||
There was a problem hiding this comment.
I believe this should probably be the more super SyntheticUIEvent<> (parent class to SyntheticMouseEvent)
src/Select/SelectInput.js
Outdated
| ); | ||
| event.persist(); | ||
| // $FlowFixMe | ||
| event.target = { ...event.target, value }; |
There was a problem hiding this comment.
Should not use $FlowFixMe. You need to implement a type refinement: https://flow.org/en/docs/lang/refinements/
That's fancy for saying you need to check first for the existence of target. It is not guaranteed to be there. Here's a link to the definition:
https://github.com/facebook/flow/blob/5c9deb498ca0b523243a8fda40160333f1f23277/lib/react.js#L408
And a flow cheatsheet (bookmark this):
https://www.saltycrane.com/flow-type-cheat-sheet/latest/
There was a problem hiding this comment.
Thanks for your help! :)
Pushed a fix, let me know if it's ok
src/Select/SelectInput.js
Outdated
| // $FlowFixMe | ||
| event.target = { ...event.target, value }; | ||
| // $FlowFixMe | ||
| this.props.onChange(event, child); |
There was a problem hiding this comment.
$FlowFixMe should not be needed, let's solve the problem. We should not introduce workarounds into a hardened codebase, I don't want to accept PRs with $FlowFixMe unless it is a last resort, which I am unconvinced it is in this case.
src/Select/SelectInput.js
Outdated
| @@ -114,14 +114,15 @@ class SelectInput extends React.Component<AllProps, State> { | |||
| }); | |||
| }; | |||
|
|
|||
| handleItemClick = (child: Element<*>) => (event: SyntheticMouseEvent<>) => { | |||
| handleItemClick = (child: Element<*>) => (event: SyntheticMouseEvent<> & { target: Object }) => { | |||
There was a problem hiding this comment.
No, you are artificially claiming that anything passed into here has a target, which is not true.
There was a problem hiding this comment.
If I understand properly, target should be optional here, and also before doing ...event.target I should check if target is present in event. Am I right?
There was a problem hiding this comment.
In the above, you are fabricating a new type using an intersection. Since we are not in control of these types, we should not be trying to alter them.
Checking presence of target is the right thing to do, since we cannot have concrete type information at this point in the handling process.
This reverts commit e02a31a.
src/Select/SelectInput.js
Outdated
| @@ -114,15 +114,21 @@ class SelectInput extends React.Component<AllProps, State> { | |||
| }); | |||
| }; | |||
|
|
|||
| handleItemClick = (child: Element<*>) => (event: SyntheticMouseEvent<>) => { | |||
| handleItemClick = (child: Element<*>) => (event: SyntheticMouseEvent<> & { target?: Object }) => { | |||
There was a problem hiding this comment.
Event is not in our control, therefore we should not be fabricating a type. It is what it is, so you must add a dynamic check (type refinement) to see if target is present.
So, this can only be event: SyntheticMouseEvent<> the way I read it.
There was a problem hiding this comment.
There already is a type refinement for target, so the last thing to do is to change target's type to any?
|
@cherniavskii you may encounter a bug I found with with flow-bin 0.55, and may require a rollback to 54.1 if I am right they don't have a fix quickly - facebook/flow#4966 Just for sanity, as you try to fix your code, you might roll the flow-bin version back to 0.54.1 just because it is a known-good version. I'm pretty confident 0.55 has a problem. |
|
@rosskevin thanks for your time! |
|
Roll back |
|
@rosskevin Actually code from my latest commit works on 55 too. Or should I use |
oliviertassinari
added
component: select
|
@rosskevin Anything preventing us from merging the PR? |
oliviertassinari
changed the title
Fixes #8381