Page MenuHomePhabricator

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

Authored by atul on Sep 12 2023, 3:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 13, 8:00 AM
Unknown Object (File)
Sat, Apr 13, 7:57 AM
Unknown Object (File)
Sat, Apr 13, 4:50 AM
Unknown Object (File)
Fri, Apr 12, 4:21 AM
Unknown Object (File)
Thu, Apr 11, 8:16 AM
Unknown Object (File)
Thu, Apr 11, 8:13 AM
Unknown Object (File)
Wed, Apr 10, 1:13 AM
Unknown Object (File)
Tue, Apr 9, 8:55 PM
Subscribers

Details

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 published this revision for review.Sep 12 2023, 3:14 PM
ginsu requested changes to this revision.Sep 12 2023, 10:00 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...

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

thanks for clarifying!

This revision is now accepted and ready to land.Sep 13 2023, 9:33 AM