feat: shopify/flash-list support

[andreialecu]

Thanks for this, I recently briefly experimented with an implementation, but I didn't finish it because reanimated's scrollTo doesn't work with Flashlist.

I'm on the phone, so I can't look too deeply into the code at the moment, but did you not run into this issue?

[andreialecu]

Or maybe it did technically seem to work, but I remember some weird errors logged.

[braccolucianoj]

Yes you are, I have just seen them. I will take care and get back to you

[braccolucianoj]

@andreialecu I saw the issue that you were talking about and I was able to spot why it was happening and fix it. I created one example Flashlist (contacts tab). The issue was related to the fact that flashlist is not directly forwarding the ref to the view underneath, it is just exposing an API and therefore the react-naive-reanimated method that needs to get the underlying view is crashing since there's nothing to match it against. The solution it is to expose to the tabs context, the recyclerlist ref (inner ref) so that it can run the animations and methods against that.

What do you think?

[andreialecu]

Looking great. I will take a look tomorrow. It would be great also to update the readme.

The procedure is to update the template and rerun the script that re-generates the docs. Careful not to update the main readme directly because it will get overwritten by the docs script.

[braccolucianoj]

Sorry I own the 2 accounts, this one is my private one 🤦 Will update them in a bit

[andreialecu]

Could you rebase please? I just updated the repo to the latest expo.

[andreialecu]

And regarding the README, would be nice to add a mention here in the Features block that FlashList is now supported: https://github.com/PedroBern/react-native-collapsible-tab-view/blob/main/documentation/README_TEMPLATE.md and then run yarn docs in root.

[andreialecu]

I just tested and it looks solid. Good work! 🚀

Should be ready to merge post-rebase and once the README nit is sorted.

[lucianobracco-geojam]

@andreialecu Everything done and updated. Let me know If I need to add something extra. Thanks!

[andreialecu]

Just realized a potential problem. This makes @shopify/flash-list a required dependency for consumers of the library, because there are direct imports to it everywhere.

It should be kept optional.

I'll follow up with some suggestions in a bit.

[lucianobracco-geojam]

Would it be enough to just set it as optional on the package.json?

[andreialecu]

I left some comments of what it might look like.

[lucianobracco-geojam]

Yes, I am applying those changes and testing on the example if it works as expected. On the helpers, I will return the flatlist if the flashlist is not added

[andreialecu]

Yes, I am applying those changes and testing on the example if it works as expected. On the helpers, I will return the flatlist if the flashlist is not added

See: https://github.com/PedroBern/react-native-collapsible-tab-view/pull/332#discussion_r1173723545

[andreialecu]

It would be best if you can test it in a project that does not have @shopify/flash-list installed. You can probably test by removing it from package.json and seeing what happens.

It's probably best to do what I mentioned here: https://github.com/PedroBern/react-native-collapsible-tab-view/pull/332#discussion_r1173718243

Inside of FlashListImpl, instead of at module level, otherwise that console.error might print every time.

[andreialecu]

To pass the typescript types forward, you can likely use import type { ... } as that shouldn't result in any runtime errors.

[lucianobracco-geojam]

Example without dependency

[andreialecu]

From what I see that error is printed at the root of the app, prior to opening the FlashList example. That would also happen for users of the library and is not optimal.

I mentioned in a previous comment that the dynamic require of the library should happen inside FlashListImpl which I think should resolve this.

[lucianobracco-geojam]

Oh yes you are right, I put outside the component definition.

[andreialecu]

It appears there's this error popping up again: https://github.com/facebook/metro/issues/666

That's probably fine. We give info to users about what's going on.

All done, or still need to do anything else? If so I think I can merge it. 🚀

[lucianobracco-geojam]

@andreialecu It is done, I will check this metro issue in the following days so we can remove that log. But this PR can be merged 🚀

[andreialecu]

I ran into that issue before and it's a metro bug, so I don't think there's anything we can do about it.

[andreialecu]

Thanks again, and great job on this.

Just released it as 6.1.0.

[lucianobracco-geojam]

Awesome! thanks for this library. I will use it right now 🚀

[andreialecu]

I'm running into some issues trying this in practice.

The content slides underneath the header and doesn't start where it should. I noticed the style and contentContainerStyle properties are not passed, I guess they were needed in certain cases.

We may need to do some additional patches here. 🤔

[andreialecu]

The issue may be that it doesn't work correctly with a dynamic header height.

I think it uses the fixed header height passed initially and doesn't adapt.

The following seems to fix that particular issue in my case:

If you have some time to investigate it would be great.

[lucianobracco-geojam]

Oh sorry, regarding that. We need to do some patches. Maybe we can work with the ListHeader. I am experiencing on a custom scenario something similar and was able to make it work by using a ListHeader (is not the best solution but is a workaround for now since this list has a lot of different configs)

[lucianobracco-geojam]

Can you leave an example so that I can test on that one and make it work on that one?

[andreialecu]

I noticed another more pressing glitch when the data property updates, the scroll jumps to the top in our app.

Here's a repro you can use on the example:

diff --git a/example/src/Shared/ContactsFlashList.tsx b/example/src/Shared/ContactsFlashList.tsx
index 9379738..859dba8 100644
--- a/example/src/Shared/ContactsFlashList.tsx
+++ b/example/src/Shared/ContactsFlashList.tsx
@@ -131,9 +131,18 @@ const Contacts: React.FC<{
 }> = ({ emptyContacts, nestedScrollEnabled }) => {
   const [isRefreshing, startRefreshing] = useRefresh()

+  const [data, setData] = React.useState(CONTACTS)
+
+  React.useEffect(() => {
+    setInterval(() => {
+      console.log('updating contacts')
+      setData([...CONTACTS])
+    }, 1000)
+  }, [])
+
   return (
     <Tabs.FlashList
-      data={emptyContacts ? [] : CONTACTS}
+      data={data}
       keyExtractor={(_, i) => String(i)}
       renderItem={renderItem}
       ItemSeparatorComponent={ItemSeparator}

https://user-images.githubusercontent.com/697707/233693256-0b3b1505-feed-46ea-bd8c-e32cd21111f0.mp4

[lucianobracco-geojam]

Have you used this property from the flatlist/flashlist:

 maintainVisibleContentPosition={{
          autoscrollToTopThreshold: 0,
          minIndexForVisible: 0,
        }}

I faced that issue and that was how i fixed it

[andreialecu]

I had to stop away, will take a look tomorrow. When I experimented with Flashlist last time I don't remember needing that though unless for special cases like chats.

Perhaps it's related to the ref callback not being memoized or something else.

[lucianobracco-geojam]

oh it could be but the ref should only be set the first time 🤔 . Will check on this example i got on a private project. That was the way we fixed that same issue.

[andreialecu]

My understanding is that Flashlist is supposed to be a drop in replacement for flatlist, where this issue doesn't exist. I also didn't notice it mentioned in the docs and couldn't find it in the issues either. That makes me think something else is going on.

[lucianobracco-geojam]

So the Flashlist is another implementation of list that uses the recyclerlist view to recycle views and make it more performant in terms of resources. FMPOV, is a normal list issue, like this issues doesn't exist as an 'issue' because there are some props to attack it (maintainVisibleContentPosition). But I will research a little more

[lucianobracco-geojam]

@andreialecu The issue is with the require, I know how to fix. I will do that right now

[lucianobracco-geojam]

@andreialecu Opened PR for this issue: https://github.com/PedroBern/react-native-collapsible-tab-view/pull/333/files

[andreialecu]

Fixed it in #334 and included the fix for the other issue I mentioned in https://github.com/PedroBern/react-native-collapsible-tab-view/pull/332#issuecomment-1518052865 - it wasn't about header height but about allowHeaderOverscroll.

There's still one issue remaining in #335 🤔