Page MenuHomePhabricator

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

Authored by atul on Sep 13 2023, 10:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Jul 11, 10:33 PM
Unknown Object (File)
Wed, Jul 10, 2:47 PM
Unknown Object (File)
Tue, Jul 9, 5:36 PM
Unknown Object (File)
Tue, Jul 9, 4:57 PM
Unknown Object (File)
Mon, Jul 8, 2:33 AM
Unknown Object (File)
Sun, Jul 7, 1:32 PM
Unknown Object (File)
Wed, Jul 3, 9:33 AM
Unknown Object (File)
Mon, Jul 1, 5:28 AM
Subscribers
None

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul published this revision for review.Sep 13 2023, 10:31 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
ashoat requested changes to this revision.Sep 13 2023, 11:01 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
native/chat/chat-thread-list.react.js
524–547 ↗(On Diff #31079)

(Actually it can only be decomposed into two)

native/chat/chat-thread-list.react.js
524–547 ↗(On Diff #31079)

On third thought, it can be three:

  1. One that only depends on hardwareBack
  2. A second that only depends on navigation (the "navigation blur listener")
  3. A third that depends on both navigation and onTabPress
native/chat/chat-thread-list.react.js
524–547 ↗(On Diff #31079)

Yeah, can break them down into three useEffects. 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.

address @ashoat's feedback: split into three separate useEffects

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