[native] Convert `ConnectedMessage` class component to functional `Message`
ClosedPublic
Actions

Authored by atul on Aug 31 2023, 8:46 AM.

Details

Reviewers

ginsu
rohan
tomek

Commits

rCOMMdb25255e8d98: [native] Convert `ConnectedMessage` class component to functional `Message`

Summary

Based on profiling of "navigating to thread w/ varun" scenario, the re-renders of Message take ~100ms. There's a few nested class components here, so converting to functional to try to memoize things in a more specific way to avoid re-renders.

Depends on D9050

Test Plan

Message component continues to appear as expected and we're skipping render of a few child components (not in a super impactful way, however)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Aug 31 2023, 8:46 AM

Herald added a subscriber: ashoat. · View Herald TranscriptAug 31 2023, 8:46 AM

atul published this revision for review.Aug 31 2023, 8:47 AM

Harbormaster completed remote builds in B22319: Diff 30660.Aug 31 2023, 9:03 AM

atul edited the summary of this revision. (Show Details)Aug 31 2023, 9:38 AM

atul added a parent revision: D9050: [native] Add some memoization to `Timestamp` component.

atul added a child revision: D9052: [native] Convert `ComposedMessage` into a functional component.

tomek requested changes to this revision.Sep 1 2023, 3:24 AM

tomek added inline comments.

native/chat/message.react.js
52–55 ↗	(On Diff #30660)	It isn't equivalent to the original code where we were checking the prev props - is there a reason for this change?
58–60 ↗	(On Diff #30660)	This can be optimized further by checking only the prop that is necessary (we're using only `keyboardState?.dismissKeyboard`). (Might be necessary to assign `keyboardState?.dismissKeyboard` to a separate variable.)
138–149 ↗	(On Diff #30660)	It is probably easier to use `React.memo` instead of memoizing the return value by hand

This revision now requires changes to proceed.Sep 1 2023, 3:24 AM

atul requested review of this revision.Sep 1 2023, 1:06 PM

atul added inline comments.

native/chat/message.react.js
52–55 ↗	(On Diff #30660)	It isn't equivalent to the original code where we were checking the prev props - is there a reason for this change? I think it's equivalent because we have `focusedOrStartsConversation` in the dep list so `LayoutAnimation.easeInEaseOut();` should fire in the same conditions?
58–60 ↗	(On Diff #30660)	Good point! Will update with that change
138–149 ↗	(On Diff #30660)	Yeah I didn't want to wrap the whole thing in React.memo because of indentation, but I guess I could export a React.memo(Message) from this file