Page MenuHomePhabricator

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

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



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

atul published this revision for review.Wed, Sep 13, 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`.Wed, Sep 13, 10:52 AM
ashoat requested changes to this revision.Wed, Sep 13, 11:01 AM

Can you decompose into three effects?

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.Wed, Sep 13, 11:01 AM
524–547 ↗(On Diff #31079)

(Actually it can only be decomposed into two)

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
524–547 ↗(On Diff #31079)

Yeah, can break them down into three useEffects. That's kind of what I did with

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.Wed, Sep 13, 11:37 AM