[native] Lift `renderSearch` to `ConnectedChatThreadList`
ClosedPublic
Actions

Authored by atul on Sep 12 2023, 3:06 PM.

Details

Reviewers

ginsu
tomek
rohan

Commits

rCOMM2d33aa44aaf5: [native] Lift `renderSearch` to `ConnectedChatThreadList`

Summary

This diff is just one step in the process of converting ChatThreadList into a functional component. When this work is done, we will be able to avoid a lot of re-rendering and hopefully improve performance quite a bit.

NOTE: There's a lot that can be refactored/improved/optimized. This is just to make progress on converting class component to functional component.

Depends on D9164

Test Plan

ChatThreadList + search experience continue to work as expected.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Sep 12 2023, 3:06 PM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 12 2023, 3:06 PM

atul published this revision for review.Sep 12 2023, 3:14 PM

Harbormaster completed remote builds in B22559: Diff 31008.Sep 12 2023, 3:24 PM

atul added a child revision: D9171: [native] Lift `onPressItem` and `onPressSeeMoreSidebars` to `ConnectedChatThreadList`.Sep 12 2023, 3:59 PM

few questions inline

native/chat/chat-thread-list.react.js
132–133 ↗	(On Diff #31008)	`mixed`
299–301 ↗	(On Diff #31008)	Should this be deleted as well?
559 ↗	(On Diff #31008)	Does this still need to happen?

This revision now requires changes to proceed.Sep 12 2023, 10:00 PM

Also as I side note, I wonder if upgrading our animation code here to renanimated 2 or 3 api would improve performance. I know that renanimated 2-3 was built with functional components first in mind and IIRC the newer apis have a bunch of hooks to create animations in a more performant way

cc @ashoat

I agree that it would be good to migrate away from the Reanimated 1 API... given it's deprecated in Reanimated 3, we'll eventually need to migrate there anyways. Looking at animateTowards though, it seems fairly complicated... I don't think it makes sense to block @atul's work here on transforming that to the new Reanimated API. We'll probably need to have the Reanimated conversion be a monthly goal for someone...

In D9166#269944, @ashoat wrote:

I agree that it would be good to migrate away from the Reanimated 1 API... given it's deprecated in Reanimated 3, we'll eventually need to migrate there anyways. Looking at animateTowards though, it seems fairly complicated... I don't think it makes sense to block @atul's work here on transforming that to the new Reanimated API. We'll probably need to have the Reanimated conversion be a monthly goal for someone...

I know GPT is good at "translating" code from one language to another. It might be helpful for "translating" from Reanimated 1 -> 3? Obviously someone who understands Reanimated well would need to take a careful look to make sure things are correct, but might be a helpful tool that could speed things up.

I don't think it makes sense to block @atul's work here on transforming that to the new Reanimated API. We'll probably need to have the Reanimated conversion be a monthly goal for someone...

That makes sense. I created this linear task to track this migration project

https://linear.app/comm/issue/ENG-4929/migrate-reanimated-1-api-to-reanimated-2-api

atul requested review of this revision.Sep 13 2023, 7:43 AM

atul added inline comments.

native/chat/chat-thread-list.react.js
132–133 ↗	(On Diff #31008)	The props will be removed altogether in a few diffs so I think it's fine for now.
299–301 ↗	(On Diff #31008)	It is in subsequent diff w/ some of the search-related functions
559 ↗	(On Diff #31008)	Yeah it'll happen once `flatList` is moved out of the class component and we have a ref