[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

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 ↗	(On Diff #31079)	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 ↗	(On Diff #31079)	(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 Diff #31079)	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 ↗	(On Diff #31079)	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.