[NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class #21585

empyrical · 2018-10-09T04:12:20Z

This PR converts the use of NativeMethodsMixin in CheckBox.android.js, and converts it to an ES6-style class.

Test Plan:

I have tested it in RNTester, and I ran in to no issues.

Release Notes:

[GENERAL] [ENHANCEMENT] [Libraries/Components/CheckBox/CheckBox.android.js] - Convert NativeMethodsMixin to forwardedRef, converted to ES6 class

analysis-bot

Code analysis results:

eslint found some issues.

analysis-bot · 2018-10-09T04:13:33Z

Libraries/Components/CheckBox/CheckBox.android.js

 const React = require('React');
+const ReactNative = require('ReactNative');


no-unused-vars: 'ReactNative' is assigned a value but never used.

analysis-bot

Code analysis results:

eslint found some issues.

analysis-bot · 2018-10-09T04:15:22Z

Libraries/Components/CheckBox/CheckBox.android.js

 const React = require('React');
+const ReactNative = require('ReactNative');


no-unused-vars: 'ReactNative' is assigned a value but never used.

analysis-bot

Code analysis results:

eslint found some issues.

analysis-bot · 2018-10-09T04:16:12Z

Libraries/Components/CheckBox/CheckBox.android.js

 const React = require('React');
+const ReactNative = require('ReactNative');


no-unused-vars: 'ReactNative' is assigned a value but never used.

elicwhite · 2018-10-09T05:17:32Z

Libraries/Components/CheckBox/CheckBox.android.js

+|}>;
+
+type NativeProps = $ReadOnly<{|
+  ...Props,


I don't think this is true. The native component doesn't take disabled or value for example.

elicwhite · 2018-10-09T05:18:12Z

Libraries/Components/CheckBox/CheckBox.android.js

+  /**
+   * Used to get the ref for the native checkbox
+   */
+  ref?: ?Object,


I don't think we have a type for this yet in Flow but I'd rather it be $FlowIssue here than Object.

RSNara

Looks mostly good, but I'm not sure if the ref forwarding is set up correctly.

RSNara · 2018-10-10T23:02:19Z

Libraries/Components/CheckBox/CheckBox.android.js

+   * Used to get the ref for the native checkbox
+   */
+  // $FlowFixMe: Properly type the ref prop type
+  ref?: $FlowIssue,


Hmm... ref like key isn't a prop, so my first instinct would be to omit it from the flow CommonProps type. Do we really need this here?

Libraries/Components/CheckBox/CheckBox.android.js

RSNara · 2018-10-10T23:53:54Z

Libraries/Components/CheckBox/CheckBox.android.js

+  };
+
+  _setAndForwardRef = nativeRef => {
+    const {ref} = this.props;


As far as I'm aware, this won't work because ref isn't a prop (i.e: this.props.ref will be undefined). What we really need is something like this:

class CheckBox extends React.Component<Props> { _setAndForwardRef = nativeRef => { const {forwardedRef} = this.props; // ... } } CheckBox2 = React.forwardRef((props, ref) => <CheckBox {...props} forwardedRef={ref}/>);

Yeah, it has to be passed in as a prop of a different name than ref (like forwardedRef)

Libraries/Components/CheckBox/CheckBox.android.js

elicwhite · 2018-10-11T16:04:03Z

@empyrical, you did a bunch of work to validate that this ref set and forwarding work. Do you think you could add some unit tests for this component that check that behavior? It would be super valuable to set a good example for other components we will want to be updating as well.

empyrical · 2018-10-11T17:32:18Z

Do you think a new section to the RNTester page that uses its ref would cover it? (like a <Button /> that toggles the state of the checkbox, perhaps)

I have a PR coming up that includes a better way to handle these forwarded refs in classes, and would like to rebase this and address the code review points once it makes its way in. (Will also rename the ref prop to forwardedRef)

The nullthrows thing I did because the type of the ref is nullable, and Flow understandably does not like trying to access a method on it without a null check. This is also how RefreshControl does it:

react-native/Libraries/Components/RefreshControl/RefreshControl.js

Lines 159 to 161 in 95174d4

    
           nullthrows(this._nativeRef).setNativeProps({ 
        
             refreshing: this.props.refreshing, 
        
           });

And I didn't do this._nativeRef && this._nativeRef.method() because if the ref handling broke at some point, then it would be a silent failure.

elicwhite · 2018-10-11T17:47:20Z

I think you can test these things using a Jest test which would be run fast and in an automated way. I think this would be better here than an RNTester example.

empyrical · 2018-10-18T06:35:51Z

Rebased this and changed it to properly forward refs using setAndForwardRef.

elicwhite · 2018-10-18T07:03:18Z

Libraries/Components/CheckBox/CheckBox.android.js

-  },
+class CheckBox extends React.Component<Props> {
+  _nativeRef: ?React.ElementRef<CheckBoxNativeType> = null;
+  _setNativeRef = setAndForwardRef({


We should flow type setAndForwardRef to ensure that getForwardedRef and setLocalRef operate on the same ref type.

Should that be done in this PR, or a separate PR? (in case this one needs to be reverted)

Separate is best. I just noticed as part of some weirdness in how it is being used here.

RSNara

Looks good! 😁

facebook-github-bot

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

react-native-bot · 2018-11-02T17:35:04Z

@empyrical merged commit 28de61e into facebook:master.

facebook#21585) Summary: This PR converts the use of `NativeMethodsMixin` in `CheckBox.android.js`, and converts it to an ES6-style class. Pull Request resolved: facebook#21585 Reviewed By: TheSavior Differential Revision: D12827768 Pulled By: RSNara fbshipit-source-id: c113c221335e61e015a20bbb6bcff5f9837f9022

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 Oct 9, 2018

empyrical requested a review from elicwhite October 9, 2018 04:12

analysis-bot suggested changes Oct 9, 2018

View reviewed changes

empyrical force-pushed the checkbox-mixin-remove branch 2 times, most recently from 6001e2b to 44a48dd Compare October 9, 2018 04:14

analysis-bot suggested changes Oct 9, 2018

View reviewed changes

react-native-bot added Core Team labels Oct 9, 2018

elicwhite reviewed Oct 9, 2018

View reviewed changes

RSNara changed the title ~~CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class~~ [NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class Oct 9, 2018

empyrical changed the title ~~[NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class~~ [WIP][NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class Oct 11, 2018

RSNara requested changes Oct 11, 2018

View reviewed changes

empyrical force-pushed the checkbox-mixin-remove branch from 40cd9b1 to 70d3553 Compare October 18, 2018 06:21

elicwhite reviewed Oct 18, 2018

View reviewed changes

empyrical changed the title ~~[WIP][NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class~~ [NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class Oct 18, 2018

empyrical added 7 commits October 25, 2018 21:21

CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class

45a6854

Remove unused require

40c2a02

Properly type RCTCheckBox

649ea49

Clean up Flow types

4b03b03

Clean up types, properly forward ref

d0e9b50

Split out styles prop

b39d62b

Add display name to forwarded ref

61d3aca

empyrical force-pushed the checkbox-mixin-remove branch from 96a4b94 to 61d3aca Compare October 26, 2018 03:44

RSNara approved these changes Oct 29, 2018

View reviewed changes

facebook-github-bot reviewed Oct 29, 2018

View reviewed changes

facebook-github-bot closed this in 28de61e Nov 2, 2018

facebook locked as resolved and limited conversation to collaborators Nov 2, 2018

react-native-bot added the Merged This PR has been merged. label Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class #21585

[NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class #21585

empyrical commented Oct 9, 2018

analysis-bot left a comment

analysis-bot Oct 9, 2018

analysis-bot left a comment

analysis-bot Oct 9, 2018

analysis-bot left a comment

analysis-bot Oct 9, 2018

elicwhite Oct 9, 2018

elicwhite Oct 9, 2018

RSNara left a comment •

edited

Loading

RSNara Oct 10, 2018

RSNara Oct 10, 2018 •

edited

Loading

elicwhite Oct 11, 2018

elicwhite commented Oct 11, 2018

empyrical commented Oct 11, 2018

elicwhite commented Oct 11, 2018

empyrical commented Oct 18, 2018

elicwhite Oct 18, 2018

empyrical Oct 18, 2018

elicwhite Oct 18, 2018

RSNara left a comment

facebook-github-bot left a comment

react-native-bot commented Nov 2, 2018

		const React = require('React');
		const ReactNative = require('ReactNative');

[NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class #21585

[NativeMethodsMixin] CheckBox: Convert NativeMethodsMixin to forwardedRef, convert to class #21585

Conversation

empyrical commented Oct 9, 2018

Test Plan:

Release Notes:

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RSNara left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RSNara Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elicwhite commented Oct 11, 2018

empyrical commented Oct 11, 2018

elicwhite commented Oct 11, 2018

empyrical commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RSNara left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Nov 2, 2018

RSNara left a comment •

edited

Loading

RSNara Oct 10, 2018 •

edited

Loading