[native] Replace `componentDidUpdate` with `useEffect`s in `ConnectedChatThreadList`
ClosedPublic
Actions

Authored by atul on Sep 13 2023, 10:56 AM.

Details

Reviewers

ginsu
tomek
rohan
ashoat

Commits

rCOMM069f7d738f1d: [native] Replace `componentDidUpdate` with `useEffect`s in…

Summary

Replace componentDidUpdate with three useEffects that should fire in the same conditions.

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.

Depends on D9189

Test Plan

ChatThreadList + search experience continue to work as expected.

Specifically, scrollToTop behavior was working as expected.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Sep 13 2023, 10:56 AM

atul published this revision for review.Sep 13 2023, 11:07 AM

atul added 1 blocking reviewer(s): ashoat.

atul added inline comments.

native/chat/chat-thread-list.react.js
520–528	This should be equivalent to the following: On mount: The initial value of `searchCancelButtonOpen` is 0, so the `searchCancelButtonOpen.setValue(0)` in the `else` case should be a noop. This effect will also fire on subsequent changes to `isActiveOrActivating`. My understanding is that `searchCancelButtonOpen` is referentially equal throughout, so it shouldn't cause the `useEffect` to fire.
530–535	This should be equivalent to the following: On mount: The initial value of `scrollPos.current` is 0 so the condition should evaluate to `false` and nothing should happen. This effect will also fire on subsequent changes to `searchText` which is equivalent to previous behavior.
536–544	This should be equivalent to the following: On mount: The initial value of `searchStatus` is `"inactive"` so the condition should evaluate to `false` and nothing should happen. This effect will fire when `searchStatus` changes. Specifically, when it changes and the new value is `"activating"`. The previous logic checked that the previous `searchStatus` was `"inactive"`, but I think this is fine as it's only possible for `searchStatus` to change to `"activating"` "from" `"inactive"`. It's NOT possible to go from `"active"` to `"activating"`.

atul attached a referenced file: F757809: fa1071.png. (Show Details)Sep 13 2023, 11:07 AM

atul attached a referenced file: F757807: f28e55.png. (Show Details)

atul attached a referenced file: F757801: ca06f1.png. (Show Details)

atul mentioned this in D9189: [native] Lift `component[DidMount/WillUnmount]` lifecycle methods to `useEffect` in `ConnectedChatThreadList`.Sep 13 2023, 11:11 AM

Harbormaster completed remote builds in B22608: Diff 31080.Sep 13 2023, 11:14 AM

atul added a child revision: D9191: [native] Remove `ChatThreadList` class component.Sep 13 2023, 11:46 AM

ashoat accepted this revision.Sep 13 2023, 1:42 PM

ashoat added inline comments.

native/chat/chat-thread-list.react.js
536–544	Seems a bit brittle, but okay. Only request is to extract `searchStatus === 'activating'` outside the effect so we can skip running the effect when eg. `searchStatus` changes from `active` to `inactive`. Not a big deal but slight perf improvement