[createReactClass] Remove createReactClass from CameraRollView by exced · Pull Request #21619 · react/react-native

exced · 2018-10-10T01:06:13Z

Related to #21581

Remove createReactClass from CameraRollView.

Test Plan:

All flow tests succeed.
Run RNTester app, go to CameraViewExample component, scroll, update image size, groupTypes.

Release Notes:

[GENERAL] [ENHANCEMENT] [CameraRollView.js] - rm createReactClass

RSNara

Looks mostly good, but we have a few problems with our flow types.

RSNara · 2018-10-10T17:31:32Z

+  }

-  _fetch: async function(clear?: boolean) {
+  _fetch = async (clear?: boolean) => {


This doesn't need to be bound:

async _fetch(clear?: boolean) {

RSNara · 2018-10-10T17:38:44Z

   * A function that takes a single image as a parameter and renders it.
   */
-  renderImage: PropTypes.func,
+  renderImage: $FlowFixMe => React.Node,


this._fetch calls CameraRoll.getPhotos and then calls this._appendAssets after waiting for the CameraRoll.getPhotos promise to resolve. this._appendAssets groups the result into rows, and passes the rows to the data source this._ds. This eventually calls this._renderRow, which calls this._renderImage. So, we can actually figure out what this type is, and remove this $FlowFixMe.

CameraRoll.getPhotos contains the type information. (I think we should export it, if necessary).

RSNara · 2018-10-10T17:40:54Z

-    rowID: string,
-  ) {
-    var images = rowData.map(image => {
+  _renderRow = (rowData: Array<Image>, sectionID: string, rowID: string) => {


rowData isn't an array of Image components.

RSNara · 2018-10-10T17:50:45Z

+  }

-  _rowHasChanged: function(r1: Array<Image>, r2: Array<Image>): boolean {
+  _rowHasChanged(r1: Array<Image>, r2: Array<Image>): boolean {


This shouldn't be image. 😞

Also, this doesn't have to be a method on the component class.

RSNara · 2018-10-10T17:52:57Z

-    var newState: Object = {loadingMore: false};
+  _appendAssets = (data: Object) => {
+    const assets = data.edges;
+    const newState: Object = {loadingMore: false};


const newState: $Shape<State> = { loadingMore: false };

RSNara · 2018-10-10T18:15:58Z

+import type {RNTesterProps} from 'RNTesterTypes';
+
+type Props = $ReadOnly<{|
+  ...RNTesterProps,


How come you're spreading the RNTesterProps into Props?

RSNara · 2018-10-10T18:17:46Z

-  _appendAssets: function(data: Object) {
-    var assets = data.edges;
-    var newState: Object = {loadingMore: false};
+  _appendAssets = (data: Object) => {


This shouldn't be bound.

…o cameraRollView

exced · 2018-10-10T23:22:56Z

@RSNara I updated type definitions and get rid of $FlowFixMe, I indeed had to export types from CameraRoll, what do you think ?

…o cameraRollView

RSNara

Almost there, but I realized that we have a lot of $FlowFixMes in this file, which is really bad since they can suppress valid flow/js errors. I think, where possible, we should try to get rid of them, because they make flow less likely to catch errors.

RSNara · 2018-10-11T18:05:31Z

+  rendererChanged() {
+    const ds = new ListView.DataSource({rowHasChanged: rowHasChanged});
    this.state.dataSource = ds.cloneWithRows(
      // $FlowFixMe(>=0.41.0)


I think the general rule of thumb should be: If you see a $FlowFixMe, try to see if you can remove it.

Pretty sure this is suppressing some valid flow errors. For example, rowHasChanged takes an array of Image components, but this.state.assets is of type Array<GetPhotosEdge>.

RSNara · 2018-10-11T18:05:48Z

   * found when Flow v0.68 was deployed. To see the error delete this comment
   * and run Flow. */
-  UNSAFE_componentWillReceiveProps: function(nextProps: {groupTypes?: string}) {
+  UNSAFE_componentWillReceiveProps(nextProps: {groupTypes?: string}) {


Can we remove this $FlowFixMe?

Not sure how we can remove it properly though.

This is the Flow error I get :
Cannot extend property Component [1] with CameraRollView because property groupTypes is read-only in object type [2] but writable in object type [3] in the first argument of property UNSAFE_componentWillReceiveProps.

RSNara · 2018-10-11T18:18:11Z

-        heading?: number,
-        speed?: number,
-      },
+export type GetPhotosEdge = {


What do you think about renaming this to:

export type PhotoIdentifierObject = {

Or:

export type PhotoIdentifier = {

RSNara · 2018-10-11T18:19:46Z

+  },
+};
+
+export type GetPhotosReturn = {


What do you think about renaming this to:

export type PhotoIdentifierObjectsPage = {

Or:

export type PhotoIdentifiersPage = {

…o cameraRollView

exced · 2018-10-11T21:20:58Z

@RSNara I got rid of every $FlowFixMe, I don't really like the PropsObject type but I think it works

RSNara

Thanks so much! This is so much better now. 😁

Edit: What do you mean by PropsObject type?

facebook-github-bot

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

exced · 2018-10-12T00:18:07Z

The Props "sub" type representation, the actual prop types object (wrapped by $ReadOnly<>)

RSNara · 2018-10-12T18:39:42Z

Ah, that was no biggie. I fixed that on my end. 😁

RSNara · 2018-10-22T16:58:52Z

Closing manually since this has been merged.

UPDATE: rm createReactClass

275858c

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

react-native-bot added 🔶Components labels Oct 10, 2018

RSNara suggested changes Oct 10, 2018

View reviewed changes

exced added 2 commits October 10, 2018 15:32

Merge branch 'master' of https://github.com/facebook/react-native int…

bad2a02

…o cameraRollView

UPDATE: flow types

c84b563

Merge branch 'master' of https://github.com/facebook/react-native int…

54ca1b9

…o cameraRollView

RSNara suggested changes Oct 11, 2018

View reviewed changes

exced added 2 commits October 11, 2018 14:31

Merge branch 'master' of https://github.com/facebook/react-native int…

18dcaa1

…o cameraRollView

UPDATE: rm $FlowFixMe

77acac9

RSNara approved these changes Oct 11, 2018

View reviewed changes

RSNara changed the title ~~Remove createReactClass from CameraRollView~~ [createReactClass] Remove createReactClass from CameraRollView Oct 11, 2018

facebook-github-bot reviewed Oct 11, 2018

View reviewed changes

RSNara closed this Oct 22, 2018

Uh oh!

Conversation

exced commented Oct 10, 2018

Test Plan:

Release Notes:

Uh oh!

RSNara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exced commented Oct 10, 2018

Uh oh!

RSNara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exced Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exced commented Oct 11, 2018

Uh oh!

RSNara left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

exced commented Oct 12, 2018

Uh oh!

RSNara commented Oct 12, 2018

Uh oh!

RSNara commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

exced Oct 11, 2018 •

edited

Loading

RSNara left a comment •

edited

Loading