[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

magrinj · 2018-08-22T13:44:46Z

Summary

Fixes #20770

Test Plan:

I test this class in my project with immutable datas, and it's perfectly working:

                    <VirtualizedSectionList
                        sections={ sections }
                        renderItem={ this._renderItem }
                        renderSectionHeader={ this._renderSectionHeader }
                        getItem={ this.getItem }
                        getItemCount={ this.getItemCount }
                        keyExtractor={ this.keyExtractor }
                        ItemSeparatorComponent={ this._renderSeparator }
                    />

    getItem(data, key) {
        return data.get(key);
    }

    getItemCount(items) {
        return items.size || 0;
    }

    keyExtractor(item, index) {
        return String(index);
    }

Changelog:

[General] [Changed] - Externalise and handle any sort of data blob

In this PR I externalise VirtualizedSectionList and make it working with any sort of data blob, so sections can be an Immutable Map by exemple.
I do this to follow the logic of VirtualizedList.
Hoping to have made the changes correctly!

sahrens

Cool, thanks for doing this. I'll need to do some pretty extensive testing to make sure everything works before importing, but hopefully it won't take too long.

Libraries/Lists/VirtualizedSectionList.js

facebook-github-bot

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

facebook-github-bot

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

sahrens

The CircleCI failures look quite relevant too - can you fix those?

magrinj · 2018-08-24T08:07:40Z

Thanks for the review @sahrens, I take a look on the test failure, and try to solve it!

magrinj · 2018-08-27T11:06:12Z

@sahrens From what I've read the flow errors due to getItem, getItemCount and getItemParam are due because the props are inherited from parent to children, and some are no longer required in child. I saw a comment added to fix this error in VirtualizedList by Gabe Levi in version 0.44.
So I did the same thing, adding this comment:

/* $FlowFixMe - this Props seems to be missing a bunch of stuff. Remove this
 * comment to see the errors */

on the top of Props declaration.
I think for the flow inheritance of these classes, it's better to only inherit of OptionalProps by exporting them, what do you think about that ?

magrinj · 2018-08-27T11:13:53Z

@sahrens I also try to implement basic test for VirtualizedSectionList, let me know if you don't want it

sahrens · 2018-08-27T18:27:09Z

I think exporting the flow types is a good way to go - gives much more control. Do you want to do that in this PR or separately?

sahrens

This is looking great, just a few more requested changes - sorry I didn't catch them in the first round.

Libraries/Lists/VirtualizedSectionList.js

Libraries/Lists/SectionList.js

sahrens · 2018-08-27T18:37:09Z

And yes, thank you for adding the test :)

magrinj · 2018-08-28T08:47:37Z

@sahrens I made the changes, but not for the type of the props, I think it's better to be done in another PR, I try to implement it, but a lot of other errors comes out, like onScroll that is not declared anywhere in Props, but used in the the classes.
I will try to submit another PR later to achieve this :)
By the way, I hope this one is good for you, and that the changes you requested are being properly implemented.

magrinj · 2018-09-04T07:38:30Z

@sahrens My last commit as you request is ok for you ? :)

magrinj · 2018-10-31T10:12:12Z

@sahrens Any news on this ?
@javache @nicklockwood

hramos · 2019-01-15T21:41:15Z

The changes look good. I was about to merge this, but it looks like some conflicts need to be resolved first. Sorry for the trouble!

magrinj · 2019-02-27T14:30:24Z

@cpojer So I remove getItemParam prop, since getItem should be enough to handle any sort of data blob, I update the tests in consequence.
I think I've also fix the flow errors, I'm not sure if I did it in a good way, but the errors disappear :)

analysis-bot

Code analysis results:

eslint found some issues.

Libraries/Lists/VirtualizedSectionList.js

facebook-github-bot

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

cpojer · 2019-02-28T03:18:02Z

Libraries/Lists/VirtualizedSectionList.js

-  // HeaderComponent?: ?ReactClass<any>,
-  // onViewableItemsChanged?: ({viewableItems: Array<ViewToken>, changed: Array<ViewToken>}) => void,
-};
+type SectionBase = Object;


This is a regression. Could you work on putting back the proper Flow Type and running flow on the react-native repo to verify it?

The SectionBase type can't be specified, because it can handle different type of datas, like classic js object or Immutable object, by exemple the data type of VirtualizedList is any.
For SectionList component SectionBase can be specified because we know the data must be javascript object.

cpojer · 2019-02-28T03:18:27Z

Libraries/Lists/VirtualizedSectionList.js

@@ -115,7 +91,8 @@ type OptionalProps<SectionT: SectionBase> = {
   */
  refreshing?: ?boolean,
 };
-
+/* $FlowFixMe - this Props seems to be missing a bunch of stuff. Remove this


Can we fix this instead?

I'm not really an expert with complex flow typing, I can try to remove this, but I'm not sure if I'm going to fix something of fix it in a good way :/

cpojer · 2019-02-28T03:20:38Z

I think this looks good but I don't feel comfortable with relaxing the flow types. If you can fix that, I'm happy to land this PR.

Overall, I think it would be great if we could change this API to use an iterator protocol, but I assume that is something to discuss later.

magrinj · 2019-02-28T10:38:00Z

@cpojer I answer in the comment above for the flow type :)

For the iterator it's a good idea, that's should work very well for VirtualizedList, but in VirtualizedSectionList some data need to be extract from item, and an iterator protocol wouldn't be enough, I think.

cpojer · 2019-04-17T10:26:26Z

If you can rebase this PR, I can take a look at the flow types.

…on-list

magrinj · 2019-04-18T14:54:23Z

@cpojer Thanks ! :) I update the branch to the current master and fix the conflicts

facebook-github-bot

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

cpojer · 2019-04-18T16:54:46Z

Libraries/Lists/VirtualizedSectionList.js

  if (!sections) {
    return null;
  }
  let itemIdx = index - 1;
-  for (let ii = 0; ii < sections.length; ii++) {
-    if (itemIdx === -1 || itemIdx === sections[ii].data.length) {
+  for (let ii = 0; ii < props.getItemCount(sections); ii++) {


Working on this at FB. I think this is supposed to stay sections.length here because sections is always an array. I'm changing this internally, no need to do anything, just wanted to put this comment here for your information.

@cpojer Thanks ! In fact it can be something else, by exemple if you use an immutable list, to get his length the method is size and not length so it's better to keep it outside to handle immutable data

cpojer · 2019-04-18T17:39:28Z

Hey @magrinj,

I'm about to land this change from FB's side after fixing up a few things. It seems that there were a bunch of bugs in the code that I fixed, among them the tests for handles separators correctly were not actually working properly but they are now.

I made a larger change to remove getItem and getItemCount calls on sections itself altogether. I think we want to be able to use custom data structure for section.data but I prefer keeping sections as arrays themselves. This should give you most of the value that you were looking for without adding too much dynamism (one getItem function to be called on different data structures, which is hard to type and prone to errors). I hope that works for you :)

magrinj · 2019-04-18T17:52:46Z

@cpojer Thanks a lot ! Sorry for the bugs, I didn't have time to test everything again between the different merges. I understand that you want to keep sections as a basic javascript array, it will be more easy to maintain in the future, and customisation on section.data should be enough for me :)
Again thank you !

react-native-bot · 2019-04-18T19:00:05Z

This pull request was successfully merged by @magrinj in 1946aee.

^{When will my fix make it into a release? | Upcoming Releases}

magrinj added 2 commits August 22, 2018 15:36

Allow VirtualizedSectionList to handle any sort of data blob

90baadb

Externalize VirtualizedSectionList

cc92082

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 Aug 22, 2018

computeState use this, so keep using UNSAFE_componentWillReceiveProps

66c365a

react-native-bot added 🔶Lists Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Aug 22, 2018

Fix flow error getItem function

c00db10

hramos requested a review from sahrens August 22, 2018 18:44

sahrens reviewed Aug 23, 2018

View reviewed changes

Libraries/Lists/VirtualizedSectionList.js Outdated Show resolved Hide resolved

facebook-github-bot reviewed Aug 23, 2018

View reviewed changes

sahrens suggested changes Aug 23, 2018

View reviewed changes

Fix some errors

0762ef6

react-native-bot added the ‼️Large PR label Aug 27, 2018

Fix lint error & refreshing flow error

b1fe9b9

sahrens suggested changes Aug 27, 2018

View reviewed changes

Libraries/Lists/VirtualizedSectionList.js Outdated Show resolved Hide resolved

Libraries/Lists/VirtualizedSectionList.js Outdated Show resolved Hide resolved

Libraries/Lists/SectionList.js Outdated Show resolved Hide resolved

Small fix

880c4dd

hramos added 🔶Components and removed 🔶Components labels Sep 21, 2018

Update VirtualizedSectionList-test.js

0168f7a

hramos removed the Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. label Feb 26, 2019

hramos changed the title ~~[INTERNAL][ENHANCEMENT][VirtualizedSectionList] - Externalise and handle any sort of data blob~~ [VirtualizedSectionList] - Externalise and handle any sort of data blob Feb 26, 2019

hramos added Component: VirtualizedList Type: Breaking Change💥 labels Feb 26, 2019

Remove getItemParam and replace by getItem and fix flow

edebcef

analysis-bot suggested changes Feb 27, 2019

View reviewed changes

Libraries/Lists/VirtualizedSectionList.js Show resolved Hide resolved

Libraries/Lists/VirtualizedSectionList.js Show resolved Hide resolved

Run prettier

4ff6035

facebook-github-bot reviewed Feb 28, 2019

View reviewed changes

cpojer reviewed Feb 28, 2019

View reviewed changes

react-native-bot added the PR: Includes Changelog label Mar 14, 2019

hramos removed the PR: Includes Changelog label Mar 14, 2019

shergin removed their request for review March 26, 2019 17:41

magrinj added 2 commits April 18, 2019 16:47

Merge remote-tracking branch 'upstream/master' into virtualized-secti…

32a5e4c

…on-list

Merge branch 'master' into virtualized-section-list

1f00f8e

facebook-github-bot reviewed Apr 18, 2019

View reviewed changes

cpojer reviewed Apr 18, 2019

View reviewed changes

facebook-github-bot closed this in 1946aee Apr 18, 2019

react-native-bot added Merged This PR has been merged. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Apr 18, 2019

fabOnReact mentioned this pull request Feb 10, 2021

SectionList unnecessarily unmounts and mounts items when filtering (in between renders) #28919

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

magrinj commented Aug 22, 2018 •

edited

Loading

sahrens left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

sahrens left a comment

magrinj commented Aug 24, 2018

magrinj commented Aug 27, 2018

magrinj commented Aug 27, 2018

sahrens commented Aug 27, 2018

sahrens left a comment

sahrens commented Aug 27, 2018

magrinj commented Aug 28, 2018

magrinj commented Sep 4, 2018

magrinj commented Oct 31, 2018

hramos commented Jan 15, 2019

magrinj commented Feb 27, 2019

analysis-bot left a comment

facebook-github-bot left a comment

cpojer Feb 28, 2019

magrinj Feb 28, 2019 •

edited

Loading

cpojer Feb 28, 2019

magrinj Feb 28, 2019

cpojer commented Feb 28, 2019

magrinj commented Feb 28, 2019

cpojer commented Apr 17, 2019

magrinj commented Apr 18, 2019

facebook-github-bot left a comment

cpojer Apr 18, 2019

magrinj Apr 18, 2019

cpojer commented Apr 18, 2019

magrinj commented Apr 18, 2019

react-native-bot commented Apr 18, 2019

[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

Conversation

magrinj commented Aug 22, 2018 • edited Loading

Summary

Test Plan:

Changelog:

sahrens left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

sahrens left a comment

Choose a reason for hiding this comment

magrinj commented Aug 24, 2018

magrinj commented Aug 27, 2018

magrinj commented Aug 27, 2018

sahrens commented Aug 27, 2018

sahrens left a comment

Choose a reason for hiding this comment

sahrens commented Aug 27, 2018

magrinj commented Aug 28, 2018

magrinj commented Sep 4, 2018

magrinj commented Oct 31, 2018

hramos commented Jan 15, 2019

magrinj commented Feb 27, 2019

analysis-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer Feb 28, 2019

Choose a reason for hiding this comment

magrinj Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

cpojer Feb 28, 2019

Choose a reason for hiding this comment

magrinj Feb 28, 2019

Choose a reason for hiding this comment

cpojer commented Feb 28, 2019

magrinj commented Feb 28, 2019

cpojer commented Apr 17, 2019

magrinj commented Apr 18, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer Apr 18, 2019

Choose a reason for hiding this comment

magrinj Apr 18, 2019

Choose a reason for hiding this comment

cpojer commented Apr 18, 2019

magrinj commented Apr 18, 2019

react-native-bot commented Apr 18, 2019

magrinj commented Aug 22, 2018 •

edited

Loading

magrinj Feb 28, 2019 •

edited

Loading