Page MenuHomePhabricator

[web] Modify `selectedThreadColors` to preserve order of keys
ClosedPublic

Authored by atul on Apr 12 2022, 3:57 PM.
Tags
None
Referenced Files
F3356590: D3715.diff
Sat, Nov 23, 7:31 PM
Unknown Object (File)
Thu, Nov 14, 4:37 PM
Unknown Object (File)
Tue, Nov 5, 8:43 PM
Unknown Object (File)
Oct 14 2024, 1:17 AM
Unknown Object (File)
Oct 14 2024, 1:17 AM
Unknown Object (File)
Oct 14 2024, 1:17 AM
Unknown Object (File)
Oct 14 2024, 1:17 AM
Unknown Object (File)
Oct 14 2024, 1:17 AM

Details

Summary

Addresses the issue flagged by @palys-swm here: https://phabricator.ashoat.com/D3702?id=11317#inline-22176

I tried a few other ways to do this, but ran into various flow issues. This aproach works, but it seems overly complex. Will continue investigating alternative approaches.

Test Plan

flow, colors now appear in the correct order:

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Adding @ashoat as first pass reviewer since this involves flow

lib/shared/thread-utils.js
102–105 ↗(On Diff #11375)

We need this invariant because flow interprets a and b as being of type [string, mixed] (https://github.com/facebook/flow/issues/2221)

atul requested review of this revision.Apr 12 2022, 4:02 PM
tomek requested changes to this revision.Apr 13 2022, 12:48 AM

This can be achieved by using keys instead of entries:
https://flow.org/try/#0MYewdgzgLgBBCmAbewrwCYBUAWAneAhugMIiIi4QDyARgFYwC8MA3gFAwwBEALAEIAOAOwBBEVwBcMAIwAaDtwCsQ5cskwATPIC+bNqEiwEyVBhz4ipcpSkASAEqF0VMIgCeI3LgJuAPNFwASzAAcwA+JhhaOhQoADoAa3g3CAAKBWNYszwnKwpqenkASjiAWwIAB1SIJgiAbQhZOCQsrBzLMnzohoBdHqKFOIgKKFTUgiaaItrWBU58KABXXDAYAjrpHpgAWhgaDZ6AbgVtAc4yytTCYGwZ6+w6gAZ+46A

But I'm also wondering if we really need an object... is there a way to make it working with just an array? Do we use the object values in some other way than just for ordering?

This revision now requires changes to proceed.Apr 13 2022, 12:48 AM
ashoat requested changes to this revision.Apr 13 2022, 11:47 AM

Let's flip the keys/values of selectedThreadColorsObj and use $Values and Object.values

Let's flip the keys/values of selectedThreadColorsObj and use $Values

Unfortunately wasn't able to get this approach to work..

$Values<T> represents the union type of all the value types (not the values, but their types!) of the enumerable properties in an Object Type T.


const selectedThreadColorsObj = {
  'a': '4B87AA',
};

export type SelectedThreadColors = $Values<typeof selectedThreadColorsObj>;

In this case type SelectedThreadColors = string


const selectedThreadColorsObj = {
  'a': '4B87AA',
  'b': 123,
};

export type SelectedThreadColors = $Values<typeof selectedThreadColorsObj>;

In this case type SelectedThreadColors = string | number


@ashoat pointed out the values utility in lib/utils/objects.js and that did the trick

lib/shared/thread-utils.js
87 ↗(On Diff #11457)

We want to preserve the order of the colors defined in this object. We're using keys in alphabetical order so we get the Object.values in the expected order. The sort order for Object.keys, Object.values, Object.entries are not clearly defined/guaranteed, but we verified that we got the expected order experimentally.

99 ↗(On Diff #11457)

Without using this utility, Object.values(selectedThreadColorsObj would be typed as array of mixed instead of as an array of strings

102 ↗(On Diff #11457)

Note the following from the flow docs:

$Values<T> represents the union type of all the value types (not the values, but their types!) of the enumerable properties in an Object Type T.

Which would mean type SelectedThreadColors = string instead of an enum of string literals.

However, as pointed out in this StackOverflow comment, we can get an enum of string literals by wrapping selectedThreadColorsObj in Object.freeze()

Looks ok!

What about:

But I'm also wondering if we really need an object... is there a way to make it working with just an array? Do we use the object values in some other way than just for ordering?

lib/shared/thread-utils.js
87 ↗(On Diff #11457)

Why do we need to use letters instead of numbers? Are the numbers sorted incorrectly?

But I'm also wondering if we really need an object... is there a way to make it working with just an array? Do we use the object values in some other way than just for ordering?

I don't think there's a way to make it work with just an array. The reason we want to use an object is so we can use the flow $Keys utility type to essentially create an enum of string literals. I don't think there's any way to create a flow type that's an enum of string literals from an array?

The alternative is to write out the string literals twice (see below). Once for the exported array of string, and once for the type which is an enum of string literals. This may be more readable, but it's less maintainable as any change in the selected colors would need to be changed twice in the code.


const selectedThreadColors: $ReadOnlyArray<string> = ['blah', 'blah2', 'blah3', 'blah4'];
export type SelectedThreadColors = 'blah' | 'blah2' | 'blah3' | 'blah4';
lib/shared/thread-utils.js
87 ↗(On Diff #11457)

Yeah, the keys of the object need to be strings and the string "10" in javascript is less than "1" so the order is thrown off if we try to use numbers

Unfortunately, my understanding is that we do indeed need an object. The next React Native upgrade will bring us a new version of Flow that has explicit support for enums, so this stuff should get a little better.

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

I've found this SO question https://stackoverflow.com/questions/54777850/flow-using-array-values-as-type where they present two possible solutions. Not sure if they're a lot cleaner, but they avoid introducing the object keys.

But to be fair, spending too much time on it doesn't make a lot of sense.