Skip to content
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

FlatList onStartReached support #7

Closed
wants to merge 12 commits into from

Conversation

roryabraham
Copy link

@roryabraham roryabraham commented Feb 17, 2022

Summary

This pull request seeks to implement bidirectional scrolling capabilities in React Native's FlatList.

Changelog

[General][Added] - Added onStartReached prop in VirtualizedList.

Test Plan

For now, all the testing is currently being done in the FlatList-basic example app. Furthermore, it currently has only been tested on iOS.

TODOs / Known Issues:

  • With a PAGE_SIZE of 100, the maintainVisibleContentPosition prop does not function as it should.
    • The contentOffset is not increase to account for the newly-prepended list items, so onStartReached is called in a loop.
    • This is likely due to a race condition, because the problem does not exist with a PAGE_SIZE of 10 or 20.
    • The relevant code (that is supposed to adjust the contentOffset to account for newly-prepended list items) is right here
    • I verified by experimenting with the PAGE_SIZE and the height of list items that this race condition has to do with the total height of new items being added, not the number of items being added. (i.e: the same onStartReached loop occurs with a PAGE_SIZE=20 and ITEM_HEIGHT=350, instead of PAGE_SIZE=100 and ITEM_HEIGHT=72)
  • Test it on Android by integrating the code from this PR
    • The review comments from that PR seem to suggest similar problems to those that I observed in the iOS implementation.
    • I believe using an O(log n) search algorithm as is suggested here might produce a minor performance benefit, but I don't think it will address the underlying race condition. In fact, it might just make the race condition harder to reproduce.
  • Add/update automated tests for this new functionality, ideally including any changes done at the native layer.
  • I'm not sure if any changes will be needed in _updateCellsToRender and/or computeWindowedRenderLimits
  • We also need to figure out how to integrate code from this fork into Expensify/App. As long as it's just JS changes (as is the case for now), it's easy to just copy changes directly into E/App's node_modules for the sake of local development. If we need to build native code from source, it becomes somewhat more complicated. In addition, I believe react-native-web uses react-native's VirtualizedList directly, so we need to ensure that when we build our fork of react-native-web that we're also using our fork of react-native.
  • Once we figure out the above ^, we also need to test this on web.

@roryabraham
Copy link
Author

We also need to figure out how to integrate code from this fork into Expensify/App.

@kidroca Since you've become so familiar with the configuration of Expensify/App, would you be interested in investigating this piece?

@kidroca
Copy link

kidroca commented Feb 21, 2022

@kidroca Since you've become so familiar with the configuration of Expensify/App, would you be interested in investigating this piece?

I see a few ways to go about it:

1 patch-package

  1. Checkout from Expensify/react-native fork
  2. Implement a feature or a fix
  3. If there's build step run it
  4. Copy the changes to Expensify/App/node_modules
  5. Run patch-package
  6. Commit

2 have an internal_release branch on Expensify's fork

  1. Checkout from Expensify/react-native fork
  2. Implement a feature or a fix
  3. Submit a PR against the internal_release branch
  4. If there's a build step run it manually or automatically on merge
  5. Use the hash from the internal release branch in Expensify/App package.json

These 2 strategies are typically for stuff that have a build step, but I don't see one for react-native - the change you're making is in Libraries/ if you open node_modules/react-native you would see Libraries/* and it's content being part of the published module. So it should be possible to just use the hash from this repo's (main branch) in place of react-native in Expensify/App package.json

@roryabraham
Copy link
Author

So it should be possible to just use the hash from this repo's (main branch) in place of react-native in Expensify/App package.json

@kidroca would you agree this would not be the case if we make changes in the native code? I think we are going to need some, on Android and probably iOS as well.

@kidroca
Copy link

kidroca commented Feb 23, 2022

So it should be possible to just use the hash from this repo's (main branch) in place of react-native in Expensify/App package.json

@kidroca would you agree this would not be the case if we make changes in the native code? I think we are going to need some, on Android and probably iOS as well.

Yes, I don't see anything ios specific, but there's definitely generated content for android that's ignored from the repository, but part of the node module
image

@roryabraham roryabraham closed this Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants