Page MenuHomePhabricator

[lib/native/web] switch reactions data structure from map to plain object
ClosedPublic

Authored by ginsu on Feb 23 2023, 2:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Dec 4, 1:48 AM
Unknown Object (File)
Fri, Nov 22, 1:21 AM
Unknown Object (File)
Fri, Nov 22, 1:21 AM
Unknown Object (File)
Fri, Nov 22, 1:21 AM
Unknown Object (File)
Fri, Nov 22, 1:21 AM
Unknown Object (File)
Fri, Nov 22, 1:21 AM
Unknown Object (File)
Fri, Nov 22, 1:21 AM
Unknown Object (File)
Fri, Nov 22, 1:14 AM
Subscribers

Details

Summary

In dev mode the app was warning whenever a user pressed a message that "Non-serializble values were found in the navigation state". This was caused becasue reactions were stored in a map. To fix this, I switched the data structure from a map to a plain object


Linear Task: ENG-2851

Test Plan

When a user taps on a message now the warning message about "non-serializable..." no longer shows up

Before:

After:

In addition tested on both native and web the following to make sure there were no regressions with reactions:

  • reacting to a message (text/multimedia/robotext)
  • unreacting to a message (text/multimedia/robotext)
  • viewing who has liked a message
  • sending multiple reactions to the same message

also flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 23 2023, 2:02 PM
Harbormaster failed remote builds in B16859: Diff 23038!
ginsu edited the test plan for this revision. (Show Details)

rebase

ginsu requested review of this revision.Feb 23 2023, 2:57 PM
ashoat requested changes to this revision.Feb 24 2023, 8:39 AM
ashoat added inline comments.
lib/selectors/chat-selectors.js
297 ↗(On Diff #23041)

Make this $ReadOnly

470 ↗(On Diff #23041)

If we're going to be storing an object anyways, should we use an object throughout? It would allow us to avoid this marshalling step

lib/shared/reaction-utils.test.js
24–27 ↗(On Diff #23041)

There is literally no point to constructing a Map and then immediately converting it to an object... please just use an object from the start...

native/chat/reaction-message-utils.js
48–51 ↗(On Diff #23041)

Would this be cleaner as a ternary?

web/chat/message-tooltip.react.js
124–127 ↗(On Diff #23041)

Would this be cleaner as a ternary?

This revision now requires changes to proceed.Feb 24 2023, 8:39 AM

address ashoat's comments

Perfect! Love the small extra improvements you made

This revision is now accepted and ready to land.Feb 26 2023, 4:32 AM