feat: shopify/flash-list support #332

lucianobracco-geojam · 2023-04-20T15:16:49Z

No description provided.

andreialecu · 2023-04-20T15:49:34Z

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 · 2023-04-20T15:50:35Z

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

braccolucianoj · 2023-04-20T16:01:24Z

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

braccolucianoj · 2023-04-20T18:36:07Z

@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 · 2023-04-20T18:36:50Z

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 · 2023-04-20T18:39:06Z

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

andreialecu · 2023-04-21T09:57:43Z

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

andreialecu · 2023-04-21T09:59:54Z

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 · 2023-04-21T10:15:35Z

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 · 2023-04-21T12:23:26Z

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

andreialecu · 2023-04-21T12:34:58Z

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.

package.json

lucianobracco-geojam · 2023-04-21T12:36:13Z

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

Co-authored-by: Andrei Alecu <aandrei03@gmail.com>

andreialecu · 2023-04-21T12:37:12Z

src/FlashList.tsx

@@ -0,0 +1,121 @@
+import { FlashList as SPFlashList, FlashListProps } from '@shopify/flash-list'


Suggested change

import { FlashList as SPFlashList, FlashListProps } from '@shopify/flash-list'

andreialecu · 2023-04-21T12:37:32Z

src/FlashList.tsx

+  useTabsContext,
+  useUpdateScrollViewContentSize,
+} from './hooks'
+


// Load FlashList dynamically or print a friendly error message let SPFlashList, FlashListProps try { const flashListModule = require('@shopify/flash-list') SPFlashList = flashListModule.FlashList FlashListProps = flashListModule.FlashListProps } catch (error) { if (error.code === 'MODULE_NOT_FOUND') { console.error( 'The optional dependency @shopify/flash-list is not installed. Please install it to use the FlashList component.' ) } else { throw error } }

Need also to forward the typescript types of flash-list properly here.

andreialecu · 2023-04-21T12:37:54Z

src/FlashList.tsx

+import { FlashList as SPFlashList, FlashListProps } from '@shopify/flash-list'
+import React from 'react'
+
+import { AnimatedFlashList } from './helpers'


This also needs to be updated.

I would suggest removing AnimatedFlashList from ./helpers in this case and just colocating it in the same file.

Yes, Just not do the same thing twice

andreialecu · 2023-04-21T12:38:27Z

I left some comments of what it might look like.

lucianobracco-geojam · 2023-04-21T12:41:31Z

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 · 2023-04-21T12:44:12Z

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: #332 (comment)

andreialecu · 2023-04-21T12:46:18Z

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: #332 (comment)

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

andreialecu · 2023-04-21T12:48:29Z

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

lucianobracco-geojam · 2023-04-21T14:35:43Z

Example without dependency

andreialecu · 2023-04-21T14:40:02Z

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 · 2023-04-21T14:41:09Z

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

andreialecu · 2023-04-21T15:01:40Z

It appears there's this error popping up again: facebook/metro#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 · 2023-04-21T15:22:06Z

@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 · 2023-04-21T15:26:26Z

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 · 2023-04-21T15:31:42Z

Thanks again, and great job on this.

Just released it as 6.1.0.

lucianobracco-geojam · 2023-04-21T15:32:15Z

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

andreialecu · 2023-04-21T16:06:21Z

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 · 2023-04-21T16:12:33Z

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 · 2023-04-21T16:41:22Z

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 · 2023-04-21T16:41:57Z

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

andreialecu · 2023-04-21T16:58:58Z

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}

Simulator.Screen.Recording.-.iPhone.14.-.2023-04-21.at.19.59.17.mp4

lucianobracco-geojam · 2023-04-21T17:43:04Z

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 · 2023-04-21T17:46:33Z

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 · 2023-04-21T17:47:45Z

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 · 2023-04-21T17:53:34Z

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 · 2023-04-21T17:58:04Z

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 · 2023-04-21T19:00:16Z

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

lucianobracco-geojam · 2023-04-21T20:37:04Z

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

andreialecu · 2023-04-22T08:10:21Z

Fixed it in #334 and included the fix for the other issue I mentioned in #332 (comment) - it wasn't about header height but about allowHeaderOverscroll.

There's still one issue remaining in #335 🤔

lucianobracco-geojam closed this Apr 20, 2023

lucianobracco-geojam reopened this Apr 20, 2023

lucianobracco-geojam force-pushed the main branch 3 times, most recently from 80533c2 to 8432a87 Compare April 20, 2023 15:45

lucianobracco-geojam added 5 commits April 21, 2023 09:14

feat: shopify/flash-list support

17cbe35

feat: added example for FlashList

46187ca

feat: fixed issue realted to scrollTo in Reanimated

152e03e

chore: created example to showcase flashlist

87a0f52

chore: added flashlist support

c41fe8b

lucianobracco-geojam force-pushed the main branch from 01b3c29 to c41fe8b Compare April 21, 2023 12:21

chore: updated documentation

6886838

andreialecu reviewed Apr 21, 2023

View reviewed changes

package.json Show resolved Hide resolved

Update package.json

2618bbf

Co-authored-by: Andrei Alecu <aandrei03@gmail.com>

andreialecu reviewed Apr 21, 2023

View reviewed changes

chore: fixed package.json suggestion

ba4a4b8

andreialecu reviewed Apr 21, 2023

View reviewed changes

fix: updated dependencies to keep flashlist as optional

5873bec

fix: dependency being required when using specific list implementation

98ed940

andreialecu merged commit 2ecc175 into PedroBern:main Apr 21, 2023

andreialecu mentioned this pull request May 8, 2023

Are there any plans to integrate the Masonry layout from Flashlist? #339

Closed

		@@ -0,0 +1,121 @@
		import { FlashList as SPFlashList, FlashListProps } from '@shopify/flash-list'

feat: shopify/flash-list support #332

feat: shopify/flash-list support #332

Conversation

lucianobracco-geojam commented Apr 20, 2023

andreialecu commented Apr 20, 2023

andreialecu commented Apr 20, 2023

braccolucianoj commented Apr 20, 2023

braccolucianoj commented Apr 20, 2023 • edited Loading

andreialecu commented Apr 20, 2023

braccolucianoj commented Apr 20, 2023

andreialecu commented Apr 21, 2023

andreialecu commented Apr 21, 2023 • edited Loading

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu Apr 21, 2023

Choose a reason for hiding this comment

andreialecu Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

andreialecu Apr 21, 2023

Choose a reason for hiding this comment

andreialecu Apr 21, 2023

Choose a reason for hiding this comment

andreialecu Apr 21, 2023

Choose a reason for hiding this comment

lucianobracco-geojam Apr 21, 2023

Choose a reason for hiding this comment

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023 • edited Loading

andreialecu commented Apr 21, 2023

andreialecu commented Apr 21, 2023

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023 • edited Loading

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 21, 2023 • edited Loading

lucianobracco-geojam commented Apr 21, 2023 • edited Loading

andreialecu commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

lucianobracco-geojam commented Apr 21, 2023

andreialecu commented Apr 22, 2023

braccolucianoj commented Apr 20, 2023 •

edited

Loading

andreialecu commented Apr 21, 2023 •

edited

Loading

andreialecu Apr 21, 2023 •

edited

Loading

lucianobracco-geojam commented Apr 21, 2023 •

edited

Loading

andreialecu commented Apr 21, 2023 •

edited

Loading

andreialecu commented Apr 21, 2023 •

edited

Loading

lucianobracco-geojam commented Apr 21, 2023 •

edited

Loading