Remove remaining PropTypes from and modernize Swipable* Classes #21278

empyrical · 2018-09-23T19:29:18Z

Part of: react-native-community/discussions-and-proposals#29

This PR removes all remaining PropTypes from the classes in Libraries/Experimental/SwipeableRow, and converts SwipeableRow from an old-style createReactClass to a proper subclass of React.Component.

This needed an alternative to TimerMixin, so I created a special <Timer /> component based off of it that can be used through a ref. So it can still use timers that will be automatically terminated when the component is unmounted.

Test Plan:

flow check passes.

A new test for the Timer component based off of the TimerMixin test has been added to Libraries/Components/Timer/__tests__/Timer-test.js

Release Notes:

[GENERAL] [FEATURE] [Libraries/Components/Timer/Timer.js] - Added a new Timer component based off of react-timer-mixin
[GENERAL] [FEATURE] [Libraries/Components/Timer/tests/Timer-test.js] - Added a test for the Timer component
[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableFlatList.js] - Cleaned up and exported props
[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableListView.js] - Removed proptypes and moved prop documentation to the flow types
[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableQuickActionButton.js] - Moved props to a separate declaration and exported them
[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableQuickActions.js] - Converted proptypes to flow types
[GENERAL] [ENHANCEMENT] [Libraries/Experimental/SwipeableRow/SwipeableRow.js] - Converted from createReactClass to a class component, and removed all proptypes
[GENERAL] [MINOR] [Libraries/Lists/ListView/ListView.js] - Exported props type so SwipeableFlatList can use them.
[GENERAL] [ENHANCEMENT] [RNTester/js/SwipeableListViewExample.js] - Finished ES6 modernization

…mponent

analysis-bot

eslint found some issues. You may run yarn prettier or npm run prettier to fix these.

analysis-bot · 2018-09-23T19:31:24Z

Libraries/Experimental/SwipeableRow/SwipeableRow.js

@@ -191,64 +177,73 @@ const SwipeableRow = createReactClass({
      </Animated.View>
    );


no-shadow: 'timer' is already declared in the upper scope.

pull-bot · 2018-09-23T19:31:31Z

	Warnings
⚠️	❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 709 lines.

Generated by 🚫 dangerJS

elicwhite · 2018-09-23T21:19:28Z

Awesome!

I believe @yungsters already had a solution for TimerMixin and many things that used to use TimerMixin have already been moved away. Can you decouple those changes from this PR?

elicwhite · 2018-09-23T21:21:35Z

Libraries/Experimental/SwipeableRow/SwipeableFlatList.js

 const React = require('React');
 const SwipeableRow = require('SwipeableRow');
 const FlatList = require('FlatList');

-type SwipableListProps = {
+type Props = {


Can you make this $ReadOnly and exact as well?

type Props = $ReadOnly<{||}>;

elicwhite · 2018-09-23T21:23:14Z

Libraries/Experimental/SwipeableRow/SwipeableFlatList.js

  renderQuickActions: renderItemType,
 };

-type Props<ItemT> = SwipableListProps & FlatListProps<ItemT>;
+export type SwipableListProps<ItemT> = Props & FlatListProps<ItemT>;


Does this need to be exported?

Can reverse this.

elicwhite · 2018-09-23T21:23:21Z

Libraries/Experimental/SwipeableRow/SwipeableFlatList.js


-type State = {
+type State = {|


elicwhite · 2018-09-23T21:24:48Z

Libraries/Experimental/SwipeableRow/SwipeableListView.js

+    rowData: any,
+    sectionID: string,
+    rowID: string,
+  ) => React.Element<any>,


Should this be React.Element or React.Node? I'm not sure what this function can return. It probably is a React.Element. 👍

I had a look around the codebase and for callbacks like this it seems to use Element

elicwhite · 2018-09-23T21:26:26Z

Libraries/Experimental/SwipeableRow/SwipeableQuickActions.js

 const React = require('React');
 const StyleSheet = require('StyleSheet');
 const View = require('View');

+import type {ViewStyleProp} from 'StyleSheet';
+
+export type SwipeableQuickActionsProps = $ReadOnly<{|


Does this need to be exported? We try not to export a bunch of things, only the things that are actually necessary or expected to be used by other files.

That makes sense! Reducing the public api surface area is a good thing.

elicwhite · 2018-09-23T21:27:56Z

Libraries/Experimental/SwipeableRow/SwipeableQuickActions.js

-
+class SwipeableQuickActions extends React.Component<
+  SwipeableQuickActionsProps,
+> {
  render(): React.Node {
    // $FlowFixMe found when converting React.createClass to ES6


While typing this component, can you clean up these FlowFixMes as well, as best as possible. This one for example is because children isn't defined in the props: children: React.Node

elicwhite · 2018-09-23T21:29:04Z

Libraries/Experimental/SwipeableRow/SwipeableRow.js

  children?: ?React.Node,
  isOpen?: ?boolean,
-  maxSwipeDistance?: ?number,
+  maxSwipeDistance: number,


Why this change?

After I converted it to a React class, it being nullable had a lot of type errors like this:

undefined [1] is not a number. [1] 59│ maxSwipeDistance?: ?number, : 306│ const maxSwipeDistance = IS_RTL 307│ ? -this.props.maxSwipeDistance 308│ : this.props.maxSwipeDistance; 309│ this._animateTo(-maxSwipeDistance, duration); 310│ } 311│ 312│ _animateToClosedPosition(duration: number = SWIPE_DURATION): void {

But it has a default prop for maxSwipeDistance, so I removed the nullability because the flow docs mention this:

Note: You don’t need to make foo nullable in your Props type. Flow will make sure that foo is optional if you have a default prop for foo.

When it's a creatReactClass again, I should revert this because that notice will no longer be relevant.

elicwhite · 2018-09-23T21:30:38Z

RNTester/js/SwipeableListViewExample.js

-    };
-  },
+class SwipeableListViewSimpleExample extends React.Component<
+  $FlowFixMeProps,


Can we do better than this?

elicwhite · 2018-09-23T21:31:20Z

RNTester/js/SwipeableListViewExample.js

-var LOREM_IPSUM =
-  'Lorem ipsum dolor sit amet, ius ad pertinax oportere accommodare, an vix civibus corrumpit referrentur. Te nam case ludus inciderint, te mea facilisi adipiscing. Sea id integre luptatum. In tota sale consequuntur nec. Erat ocurreret mei ei. Eu paulo sapientem vulputate est, vel an accusam intellegam interesset. Nam eu stet pericula reprimique, ea vim illud modus, putant invidunt reprehendunt ne qui.';
+
+const LOREM_IPSUM = [


Could also use template literal

elicwhite · 2018-09-23T21:33:00Z

This is great. Thanks for cleaning up / updating these files! For the sections that might require more back and forth or are less obvious on the approach, splitting those changes out can let us merge the rest of it and mark them off the list as completed!

empyrical · 2018-09-23T21:39:25Z

For the TimerMixin thing, I will have to revert it back to a createReactClass for now. I tried looking around and seeing what was being used as an alternative to TimerMixin before I did that, but I couldn't see it.

empyrical · 2018-09-23T21:43:27Z

I have made a copy of the current version of this branch so my Timer component is more easily referenced to once I update this PR to the feedback:

https://github.com/empyrical/react-native/tree/swipable-updating-bak/Libraries/Components/Timer

elicwhite · 2018-09-23T22:05:42Z

Apparently @yungsters's approach was to just inline the timer:

this._timerID = setTimeout(…);

And then in componentWillUnmount:

if (this._timerID != null) {
  clearTimeout(this._timerID);
}

It seems simple enough and easier to read than having a whole component. What do you think?

empyrical · 2018-09-23T23:18:37Z

I am going to redo this again as separate, smaller pull requests.

Remove remaining PropTypes from Swipable* classes, introduce Timer co…

231641d

…mponent

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 23, 2018

analysis-bot suggested changes Sep 23, 2018

View reviewed changes

empyrical requested a review from elicwhite September 23, 2018 19:32

react-native-bot added Core Team labels Sep 23, 2018

empyrical mentioned this pull request Sep 23, 2018

Deprecate and eventually remove propTypes from core react-native-community/discussions-and-proposals#29

Closed

Fix 'no-shadow' lint error

22f412d

elicwhite requested changes Sep 23, 2018

View reviewed changes

empyrical closed this Sep 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove remaining PropTypes from and modernize Swipable* Classes #21278

Remove remaining PropTypes from and modernize Swipable* Classes #21278

empyrical commented Sep 23, 2018

analysis-bot left a comment

analysis-bot Sep 23, 2018

pull-bot commented Sep 23, 2018

elicwhite commented Sep 23, 2018

elicwhite Sep 23, 2018

elicwhite Sep 23, 2018

empyrical Sep 23, 2018

elicwhite Sep 23, 2018

elicwhite Sep 23, 2018

empyrical Sep 23, 2018

elicwhite Sep 23, 2018

empyrical Sep 23, 2018

elicwhite Sep 23, 2018

elicwhite Sep 23, 2018

empyrical Sep 23, 2018

elicwhite Sep 23, 2018

elicwhite Sep 23, 2018

elicwhite commented Sep 23, 2018

empyrical commented Sep 23, 2018

empyrical commented Sep 23, 2018

elicwhite commented Sep 23, 2018 •

edited

Loading

empyrical commented Sep 23, 2018 •

edited

Loading

		@@ -191,64 +177,73 @@ const SwipeableRow = createReactClass({
		</Animated.View>
		);

Remove remaining PropTypes from and modernize Swipable* Classes #21278

Remove remaining PropTypes from and modernize Swipable* Classes #21278

Conversation

empyrical commented Sep 23, 2018

Test Plan:

Release Notes:

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pull-bot commented Sep 23, 2018

elicwhite commented Sep 23, 2018

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

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

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

Choose a reason for hiding this comment

elicwhite commented Sep 23, 2018

empyrical commented Sep 23, 2018

empyrical commented Sep 23, 2018

elicwhite commented Sep 23, 2018 • edited Loading

empyrical commented Sep 23, 2018 • edited Loading

elicwhite commented Sep 23, 2018 •

edited

Loading

empyrical commented Sep 23, 2018 •

edited

Loading