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

[FlowCleanup] Add flow types to PanResponder #21291

Closed
wants to merge 4 commits into from

Conversation

empyrical
Copy link
Contributor

This PR adds flow types to the PanResponder module. It is part of my effort to flowtype the Swipable* classes.

A new touchHistory field had to be added to SyntheticEvent as well.

Test Plan:

Flow check and running the whole jest test suite showed no signs of regressions.

Release Notes:

[GENERAL] [ENHANCEMENT] [Libraries/Interaction/PanResponder.js] - Added flow type annotations, and exported the GestureState type which is used by callbacks.
[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableRow.js] - Flow typed SwipableRow's use of PanResponder
[GENERAL] [ENHANCEMENT] [Libraries/Types/CoreEventTypes.js] - Added a new touchHistory field to SyntheticEvent

@empyrical
Copy link
Contributor Author

Looks like tests are failing because I rebased it against master after Metro was bumped, and that version of Metro's not on NPM yet. Will need to rerun these when that's resolved.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 24, 2018
_updateGestureStateOnMove: function(gestureState, touchHistory) {
_updateGestureStateOnMove(
gestureState: GestureState,
touchHistory: $PropertyType<PressEvent, 'touchHistory'>,
Copy link
Member

Choose a reason for hiding this comment

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

😉 ❤️

@@ -26,6 +26,12 @@ export type SyntheticEvent<T> = $ReadOnly<{|
persist: () => void,
target: ?number,
timeStamp: number,
touchHistory: $ReadOnly<{|
Copy link
Member

Choose a reason for hiding this comment

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

Is this always set? Like, does View's onLayout callback include touchHistory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure - I tried a few events out, and they all seemed to have it - but those were all touch-related, so that it probably why. I will make it nullable, but will require some more modifications to panresponder

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is where it is set:

/**
* `touchHistory` isn't actually on the native event, but putting it in the
* interface will ensure that it is cleaned up when pooled/destroyed. The
* `ResponderEventPlugin` will populate it appropriately.
*/
var ResponderSyntheticEvent = SyntheticEvent.extend({
touchHistory: function(nativeEvent) {
return null; // Actually doesn't even look at the native event.
}
});

Copy link
Member

Choose a reason for hiding this comment

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

Want to add a matching ResponderSyntheticEvent to CoreEventTypes?

export type ResponderSyntheticEvent<T> = $ReadOnly<{|
  ...SyntheticEvent<T>
  // touchHistory...
|}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that, and it works great! 👍

@elicwhite
Copy link
Member

This is amazing, thank you so much for taking on Flow typing PanResponder. This will be a great win for type safety!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

TheSavior has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

const panHandlers = {
onStartShouldSetResponder: function(e) {
return config.onStartShouldSetPanResponder === undefined
onStartShouldSetResponder(event: PressEvent) {
Copy link
Member

Choose a reason for hiding this comment

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

If you type this as returning a boolean I think you'll find that the flow type for onStartShouldSetPanResponder is not correct.

Same with onStartShouldSetResponderCapture and onResponderTerminationRequest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 will take a look at this. Also I realize that the type for touchHistory.touchBank is wrong, so I will fix that too.

@empyrical
Copy link
Contributor Author

Added return types to everything in panHandlers. Also, I made two different callback types - ActiveCallback, which returns a boolean, and PassiveCallback which is mixed. Curious about feedback on the naming of them/if they're okay.

Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

On the whole, this is really awesome! What do you think of the comments I left?

@@ -121,6 +124,81 @@ const currentCentroidY = TouchHistoryMath.currentCentroidY;
* [PanResponder example in RNTester](https://github.com/facebook/react-native/blob/master/RNTester/js/PanResponderExample.js)
*/

export type GestureState = {|
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing a $ReadOnly here?

Copy link
Contributor Author

@empyrical empyrical Sep 25, 2018

Choose a reason for hiding this comment

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

Several places in PanResponder change the values of gestureState, so setting this to $ReadOnly raises many errors

* ID of the gestureState - persisted as long as there at least one touch on screen
*/
stateID: number,
/**
Copy link
Contributor

@RSNara RSNara Sep 25, 2018

Choose a reason for hiding this comment

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

nit: I think it'd be a good idea to space out the comment from the property above it. It visually groups the comment with the property, which makes it easier (for at least me) to read this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spaced them out

*/
numberActiveTouches: number,
/**
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you forgot to put a description here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

const panHandlers = {
onStartShouldSetResponder: function(e) {
return config.onStartShouldSetPanResponder === undefined
onStartShouldSetResponder(event: PressEvent): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's better to add the type annotation to panHandlers instead:

const panHandlers: {[key: string]: (event: PressEvent) => boolean} = {
  onStartShouldSetResponder(event) {
    // ...
  }
}

With this approach, you only have to type once for the whole map, as opposed to once per every function in it. Also, if someone adds a function to this map that doesn't conform to the panHandler signature, flow will raise an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all of the functions have the same signature (see the types in PanResponderConfig) so this wouldn't work (unless I had it be a type like: {[key: string]: ActiveCallback | PassiveCallback})

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

rsnara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@empyrical merged commit 3f79b2a into facebook:master.


Once this commit is added to a release, you will see the corresponding version tag below the description at 3f79b2a. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 27, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 27, 2018
@hramos hramos changed the title Add flow types to PanResponder [FlowCleanup] Add flow types to PanResponder Oct 1, 2018
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
Summary:
This PR adds flow types to the `PanResponder` module. It is part of my effort to flowtype the `Swipable*` classes.

A new `touchHistory` field had to be added to `SyntheticEvent` as well.
Pull Request resolved: facebook#21291

Reviewed By: TheSavior

Differential Revision: D10013265

Pulled By: RSNara

fbshipit-source-id: 3cd65a0eae41c756d1605e6771588d820f040e2a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API: PanResponder CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants