[lib] Create util function for parsing locally unique thread ID
ClosedPublic
Actions

Authored by • jacek on Jul 5 2022, 7:57 AM.

Details

Reviewers

tomek
atul

Commits

rCOMM6f183e6bbeee: [lib] Create util function for parsing locally unique thread ID

Summary

Created util function that is reversed version of getLocallyUniqueThreadID.
Basing on provided pending thread ID, it parses all data, that could be read from it - source message ID, thread type, and user ID list.

The goal of the function is to support navigation so that pending threads could be loaded, basing on URL (e.g. after page refresh),

Test Plan

The function is not used yet. Tested the feature after introducing further navigation changes.
Introduced automatic tests, that could be run with cd lib && yarn test

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• jacek created this revision.Jul 5 2022, 7:57 AM

Herald added subscribers: • abosh, • adrian, ashoat. · View Herald TranscriptJul 5 2022, 7:57 AM

• jacek edited the summary of this revision. (Show Details)Jul 5 2022, 8:01 AM

Harbormaster completed remote builds in B10295: Diff 14193.Jul 5 2022, 8:10 AM

• jacek requested review of this revision.Jul 5 2022, 8:10 AM

I'm surprised we don't have any existing code that needs to parse this info out of a threadID. Would be great if we could 100% make sure of this before landing! cc @palys-swm

Thanks for including the tests, they made usage of the function much clearer (could easily set breakpoints and step through it!)

Outside the scope of this diff, but I wonder how robust our approach of encoding this data into a string is? I wonder if JSON.stringify(...)ing some object would be a better way to encode things?

Because right now we depend on kind of brittle logic (eg splitting on / and +) and assumptions (eg userIDs will never contain + or /)? Like *maybe* one day one of these values is encrypted and the ciphertext representation contains /s and +s and then things break?

Would be great if we could 100% make sure of this before landing! cc @palys-swm

(should still check this before landing)

lib/shared/thread-utils.js
278–282 ↗	(On Diff #14193)	Might be good to pull this out into a new type? Something like type LocallyUniqueThreadIDContents = { +threadType: ThreadType, +memberIDs: $ReadOnlyArray<string>, +sourceMessageID: ?string, };
283–285 ↗	(On Diff #14193)	Should we have some sort of precondition that we enforce to ensure that `pendingThreadID.startsWith("pending")` or something?

This revision is now accepted and ready to land.Jul 5 2022, 2:14 PM

atul added inline comments.Jul 5 2022, 2:20 PM

lib/shared/thread-utils.test.js
50 ↗	(On Diff #14193)	nit: `invarian` => `invariant`

• jacek added a child revision: D4455: [web] Introduce pending thread creation in `websiteResponder` if it is missing.Jul 6 2022, 3:16 AM

In D4446#126356, @ashoat wrote:

I'm surprised we don't have any existing code that needs to parse this info out of a threadID. Would be great if we could 100% make sure of this before landing! cc @palys-swm

It seems like we didn't need that - we're using threadInfo when determining e.g. other users of a thread. But still, it might be a good idea to check that.

lib/shared/thread-utils.js
273–274 ↗	(On Diff #14193)	Is there a way to use this regex (match and get group captures) when parsing the id? By doing that we would make sure that it doesn't go out of sync with the logic.
283–285 ↗	(On Diff #14193)	Our experience shows that `invariants` should be avoided, because they could cause a lot of issues during runtime - they should only express things that are critical to our logic but can't be expressed as types. In this case, returning `null` sounds good enough - we don't need anything stronger.

• jacek added inline comments.Jul 19 2022, 8:29 AM

lib/shared/thread-utils.js
273–274 ↗	(On Diff #14193)	I think it's a good idea. I'll check if it's possible to use it

use pending thread ID regex in function to check if provided ID is valid.

• jacek requested review of this revision.Jul 20 2022, 7:13 AM

Harbormaster completed remote builds in B10688: Diff 14713.Jul 20 2022, 7:24 AM

Looks good!

(Cool thing about including the unit tests is they increase confidence that the changes between diff iterations are correct)

This revision is now accepted and ready to land.Jul 20 2022, 9:52 AM

atul added inline comments.Jul 20 2022, 9:58 AM

lib/shared/thread-utils.js
298 ↗	(On Diff #14713)	Hm, could you explain what's going on here? Guessing that `threadKey` looks something like: `12345+67890+98765+43210` After splitting on `('+')`, we'd get something like: `["12345", "67890", "98765", "43210"]` And then we're comparing the value of each element (type `string`) with its length (type `number`)? I'm probably missing something here

• jacek added inline comments.Jul 20 2022, 10:18 AM

lib/shared/thread-utils.js
298 ↗	(On Diff #14713)	It just removes empty strings from list - so if someone enters a string like `12345+67890+98765++++43210` in URL (what is allowed by the regex), the array will be `["12345", "67890", "98765", "43210"]` instead `["12345", "67890", "98765", "", "", "", "43210"]`

atul added inline comments.Jul 20 2022, 10:51 AM

lib/shared/thread-utils.js
298 ↗	(On Diff #14713)	Gotcha, is there another way we can do this that's more obvious? Maybe filter out based on truthiness? Maybe we can enforce things directly with the regex? (cc @yayabosh @ashoat who iirc are pretty familiar with regex)

tomek added inline comments.Jul 21 2022, 1:49 AM

lib/shared/thread-utils.js
298 ↗	(On Diff #14713)	Maybe we can modify the regex to avoid strings with repeated plus signs `12345+67890+98765++++43210`? So the regex for id list could look like this: `[0-9]+(+[0-9]+)*`