[native] Lift `component[DidMount/WillUnmount]` lifecycle methods to `useEffect` in `ConnectedChatThreadList`
ClosedPublic
Actions

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

Details

Reviewers

ginsu
tomek
rohan
ashoat

Commits

rCOMMfc4af3014583: [native] Lift `component[DidMount/WillUnmount]` lifecycle methods to…

Summary

Replace componentDidMount and componentWillUnmount with a useEffect in ConnectedThreadList. These lifecycle methods effectively add and remove listeners.

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 D9188

Test Plan

ChatThreadList + search experience continue to work 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:28 AM

atul published this revision for review.Sep 13 2023, 10:31 AM

Harbormaster completed remote builds in B22607: Diff 31079.Sep 13 2023, 10:45 AM

atul retitled this revision from [native] Lift `component[DidMount/WillUnmount]` lifestyle methods to `useEffect` in `ConnectedChatThreadList` to [native] Lift `component[DidMount/WillUnmount]` lifecycle methods to `useEffect` in `ConnectedChatThreadList`.Sep 13 2023, 10:52 AM

atul added a child revision: D9190: [native] Replace `componentDidUpdate` with `useEffect`s in `ConnectedChatThreadList`.Sep 13 2023, 10:56 AM

Can you decompose into three effects?

native/chat/chat-thread-list.react.js
524–547	The effect version of this is a little different because it will rerun if any of its inputs change. This isn't necessarily a bad thing... we should probably re-add these listeners in that case. But it would be a good idea to decompose this into three effects to avoid unnecessary re-mounting of unrelated listeners

This revision now requires changes to proceed.Sep 13 2023, 11:01 AM

ashoat added inline comments.Sep 13 2023, 11:02 AM

native/chat/chat-thread-list.react.js
524–547	(Actually it can only be decomposed into two)

ashoat added inline comments.Sep 13 2023, 11:04 AM

native/chat/chat-thread-list.react.js
524–547	On third thought, it can be three: One that only depends on `hardwareBack` A second that only depends on `navigation` (the "navigation blur listener") A third that depends on both `navigation` and `onTabPress`

atul added inline comments.Sep 13 2023, 11:11 AM

native/chat/chat-thread-list.react.js
524–547	Yeah, can break them down into three `useEffect`s. That's kind of what I did with https://phab.comm.dev/D9190 Specifically it looks like `hardwareBack` has `searchStatus` in the dependency list which would cause all of these listeners to be re-mounted. Will address feedback and update diff.