[lib][native][web] Introduce EntityText
ClosedPublic
Actions

Authored by ashoat on Feb 1 2023, 2:03 PM.

Details

Reviewers

atul
tomek
ginsu

Commits

rCOMM5b7efed3c974: [lib][native][web] Introduce EntityText

Summary

For the ENS work, we need to be able to "introspect" into robotext, find the usernames, and swap them out. This is much easier to do when have an AST to deal with instead of a string.

This will also allow us to get rid of global_viewer in favor of having the messageTitle code just go through the AST and set isViewer to false everywhere.

In future diffs I'll update notifTexts, messageTitle, and uiName (as well as robotext) to all use this new EntityText framework. We need to swap in ENS names in all of these cases.

Test Plan

There's a small unit test here, but mostly this has been tested in combination with the following diffs

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.Feb 1 2023, 2:03 PM

ashoat added a parent revision: D6506: [lib] Add messageInfoType field to ChatMessageInfoItem.Feb 1 2023, 2:04 PM

ashoat edited the test plan for this revision. (Show Details)

ashoat added a child revision: D6509: [lib] Add helpers for EntityText.Feb 1 2023, 2:05 PM

Harbormaster completed remote builds in B15967: Diff 21803.Feb 1 2023, 2:20 PM

ashoat requested review of this revision.Feb 1 2023, 2:20 PM

ashoat added inline comments.Feb 1 2023, 3:00 PM

lib/utils/entity-text.js
85	I should return immediately here

Clean up getNameForThreadEntity

Harbormaster completed remote builds in B15980: Diff 21821.Feb 1 2023, 5:54 PM

Looks complicated but correct. Reviewing this thoroughly might take more time, but I haven't found any issues yet.

lib/utils/entity-text.js
87 ↗	(On Diff #21821)
142–144 ↗	(On Diff #21821)	Can we use index instead?
native/chat/inner-robotext-message.react.js
51–63 ↗	(On Diff #21821)	Maybe if we type the functions as components, we can simplify this code? I'm thinking about a solution where we can simply write `renderThread: ThreadEntity`.

This revision is now accepted and ready to land.Feb 2 2023, 6:10 AM

ashoat added inline comments.Feb 2 2023, 6:26 AM

lib/utils/entity-text.js
87 ↗	(On Diff #21821)	This won't work – Flow can't refine the type of `entity` unless the `invariant` is explicitly on `entity`
128 ↗	(On Diff #21821)	This is joining with commas – it should've been passed an empty string
142–144 ↗	(On Diff #21821)	Yeah, that's a way better idea
native/chat/inner-robotext-message.react.js
51–63 ↗	(On Diff #21821)	The downsides are: We'd have to increase the depth of the component hierarchy We'd have to introduce a new component for `renderText` I think the downsides outweigh the upsides personally... I don't see this change improving readability all that much