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.

Sign up for GitHub

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

Jump to bottom

[flow-strict] Flow TouchableNativeFeedback.android.js #22176

Closed
wants to merge 9 commits into from
Closed

[flow-strict] Flow TouchableNativeFeedback.android.js #22176

wants to merge 9 commits into from

Conversation

mottox2
Copy link
Contributor

@mottox2 mottox2 commented Nov 6, 2018 •
edited
Loading

Related to #22100

Turn Flow strict mode on for Libraries/Components/Touchable/TouchableNativeFeedback.android.js.

Test Plan:

  • npm run prettier
  • npm run flow
  • npm run flow-check-ios
  • npm run flow-check-android
  • npm run lint
  • npm run test

Changelog:

[GENERAL][FIXED] - apply flow

@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 Nov 6, 2018
@pull-bot
Copy link

pull-bot commented Nov 6, 2018

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

@mottox2 mottox2 changed the title [flow] Flow TouchableNativeFeedback.android.js [WIP] [flow] Flow TouchableNativeFeedback.android.js Nov 6, 2018
@mottox2
Copy link
Contributor Author

mottox2 commented Nov 6, 2018 •
edited
Loading

Sorry. I will submit PR to apply flow strict to Touchable.js before this PR.

I use //$FlowFixMe to import Touchable.js.

@mottox2 mottox2 changed the title [WIP] [flow] Flow TouchableNativeFeedback.android.js [flow] Flow TouchableNativeFeedback.android.js Nov 6, 2018
@mottox2 mottox2 changed the title [flow] Flow TouchableNativeFeedback.android.js [flow-strict] Flow TouchableNativeFeedback.android.js Nov 6, 2018
elicwhite
elicwhite reviewed Nov 6, 2018
View reviewed changes
Libraries/Components/Touchable/TouchableNativeFeedback.android.js Outdated
@@ -13,6 +14,7 @@ const Platform = require('Platform');
const React = require('React');
const PropTypes = require('prop-types');
const ReactNative = require('ReactNative');
// $FlowFixMe
Copy link
Member

Choose a reason for hiding this comment

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

This is to fix the flow warning about not importing from untyped files, right? If that is the case then lets just not turn on strict-local in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing.
I applied flow to Touchable.js then I removed FlowFixMe from TouchableNativeFeedback.android.js.

@mottox2 mottox2 force-pushed the flow-touchable-native-feedback branch from 1996e49 to 4623d51 Compare November 7, 2018 01:48
RSNara
RSNara approved these changes Nov 12, 2018
View reviewed changes
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.

Looks good! 👍🏽

facebook-github-bot
facebook-github-bot reviewed Nov 12, 2018
View reviewed changes
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.

RSNara
RSNara reviewed Nov 21, 2018
View reviewed changes
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.

@mottox2: I noticed a few contradictions in this diff while I was reviewing it internally. You used LayoutEvent in interesting ways. (I've fixed up the diff internally to only use PressEvent, so you don't need to push any updates to this). Before I landed the diff, I just wanted to check in here to see if I was missing something. 😅

Libraries/Components/Touchable/Touchable.js
@@ -652,12 +698,12 @@ const TouchableMixin = {
);
},

_handleDelay: function(e) {
_handleDelay: function(e: LayoutEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit curious. How come you decided to type this with LayoutEvent?

Libraries/Components/Touchable/Touchable.js
this.touchableDelayTimeout = null;
this._receiveSignal(Signals.DELAY, e);
},

_handleLongDelay: function(e) {
_handleLongDelay: function(e: LayoutEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here. Why is this a LayoutEvent and not a PressEvent? This function calls _receiveSignal, which I think should accept a PressEvent.

Libraries/Components/Touchable/Touchable.js
return (
state === States.RESPONDER_ACTIVE_PRESS_IN ||
state === States.RESPONDER_ACTIVE_LONG_PRESS_IN
);
},

_savePressInLocation: function(e) {
_savePressInLocation: function(e: LayoutEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Later on in the function, we call TouchEventUtils.extractSingleTouch, which expects to be called with the native event of a PressEvent. So, I think this type should be PressEvent and not LayoutEvent.

Libraries/Components/Touchable/Touchable.js
this._savePressInLocation(e);
this.touchableHandleActivePressIn && this.touchableHandleActivePressIn(e);
},

_endHighlight: function(e) {
_endHighlight: function(e: LayoutEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method calls this.touchableHandleActivePressOut, which you typed as accepting a PressEvent inside TouchableNativeFeedback.android.js. So, shouldn't the type of e also be PressEvent?

Libraries/Components/Touchable/Touchable.js
@@ -813,12 +869,12 @@ const TouchableMixin = {
UIManager.playTouchSound();
},

_startHighlight: function(e) {
_startHighlight: function(e: LayoutEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal as _endHighlight here.

Libraries/Components/Touchable/Touchable.js
nextState: State,
signal: Signal,
e: LayoutEvent,
) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This event is forwarded to _startHighlight and _endHighlight. Shouldn't it therefore also be typed as a PressEvent?

Libraries/Components/Touchable/Touchable.js
@@ -685,7 +731,7 @@ const TouchableMixin = {
* @throws Error if invalid state transition or unrecognized signal.
* @sideeffects
*/
_receiveSignal: function(signal, e) {
_receiveSignal: function(signal: Signal, e: LayoutEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets forwarded to _performSideEffectsForTransition. So, shouldn't e be typed as PressEvent?

Libraries/Components/Touchable/Touchable.js
@@ -391,7 +430,7 @@ const TouchableMixin = {
* @param {SyntheticEvent} e Synthetic event from event system.
*
*/
touchableHandleResponderGrant: function(e) {
touchableHandleResponderGrant: function(e: SyntheticEvent<any>) {
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 this function accepts a PressEvent.

Libraries/Components/Touchable/Touchable.js
this._receiveSignal(Signals.RESPONDER_RELEASE, e);
},

/**
* Place as callback for a DOM element's `onResponderTerminate` event.
*/
touchableHandleResponderTerminate: function(e) {
touchableHandleResponderTerminate: function(e: PressEvent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You've typed this function as accepting a PressEvent, but later on in the function, we forward this event to _receiveSignal, which accepts a LayoutEvent. There's a contradiction here.

@react-native-bot
Copy link
Collaborator

@mottox2 merged commit 3649a50 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Nov 21, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Nov 21, 2018
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
@mottox2 @facebook-github-bot
Flow TouchableNativeFeedback.android.js (facebook#22176)
2dcc666 
Summary:
Related to facebook#22100

Turn Flow strict mode on for Libraries/Components/Touchable/TouchableNativeFeedback.android.js.
Pull Request resolved: facebook#22176

Reviewed By: TheSavior

Differential Revision: D13033239

Pulled By: RSNara

fbshipit-source-id: 57e89ce16040e6238e01e0437327db943a5f2581
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@RSNara RSNara RSNara approved these changes

@elicwhite elicwhite elicwhite left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Component: TouchableNativeFeedback Flow Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mottox2 @pull-bot @react-native-bot @elicwhite @RSNara @facebook-github-bot