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

[createReactClass] Remove createReactClass from CameraRollView #21619

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@exced
Copy link
Contributor

exced commented Oct 10, 2018

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
Copy link
Contributor

RSNara left a comment

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


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

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

This doesn't need to be bound:

  async _fetch(clear?: boolean) {

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

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

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).

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

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

rowData isn't an array of Image components.


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

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

This shouldn't be image. 😞

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

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

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

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor
const newState: $Shape<State> = { loadingMore: false };
import type {RNTesterProps} from 'RNTesterTypes';

type Props = $ReadOnly<{|
...RNTesterProps,

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

How come you're spreading the RNTesterProps into Props?

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

This comment has been minimized.

@RSNara

RSNara Oct 10, 2018

Contributor

This shouldn't be bound.

@exced

This comment has been minimized.

Copy link
Contributor Author

exced commented Oct 10, 2018

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

@RSNara
Copy link
Contributor

RSNara left a comment

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.

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

This comment has been minimized.

@RSNara

RSNara Oct 11, 2018

Contributor

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>.


/* $FlowFixMe(>=0.68.0 site=react_native_fb) This comment suppresses an error
* 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}) {

This comment has been minimized.

@RSNara

RSNara Oct 11, 2018

Contributor

Can we remove this $FlowFixMe?

This comment has been minimized.

@exced

exced Oct 11, 2018

Author Contributor

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.

Show resolved Hide resolved RNTester/js/CameraRollView.js
Show resolved Hide resolved RNTester/js/CameraRollView.js
Show resolved Hide resolved RNTester/js/CameraRollView.js
heading?: number,
speed?: number,
},
export type GetPhotosEdge = {

This comment has been minimized.

@RSNara

RSNara Oct 11, 2018

Contributor

What do you think about renaming this to:

export type PhotoIdentifierObject = {

Or:

export type PhotoIdentifier = {
},
};

export type GetPhotosReturn = {

This comment has been minimized.

@RSNara

RSNara Oct 11, 2018

Contributor

What do you think about renaming this to:

export type PhotoIdentifierObjectsPage = {

Or:

export type PhotoIdentifiersPage = {
@exced

This comment has been minimized.

Copy link
Contributor Author

exced commented Oct 11, 2018

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

@RSNara

RSNara approved these changes Oct 11, 2018

Copy link
Contributor

RSNara left a comment

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

Edit: What do you mean by PropsObject type?

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

@facebook-github-bot
Copy link

facebook-github-bot left a comment

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

@exced

This comment has been minimized.

Copy link
Contributor Author

exced commented Oct 12, 2018

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

@RSNara

This comment has been minimized.

Copy link
Contributor

RSNara commented Oct 12, 2018

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

@RSNara

This comment has been minimized.

Copy link
Contributor

RSNara commented Oct 22, 2018

Closing manually since this has been merged.

@RSNara RSNara closed this Oct 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment