presence: Modelify; use Immutable; make more extensible

[gnprice]

This is preparation for #5669.

For that issue, we'll want to store a couple more pieces of data from the server and use them in various presence-related logic. The way that logic is currently structured makes it pretty inflexible for this kind of change — it's a lot of disparate fragments of the state passed in as separate arguments to each of many different helper functions. So, here's a series of refactorings that takes us most of the way toward being able to straightforwardly add new information of the kind needed in #5669.

As a bonus, we replace the object-as-map in state.presence with an Immutable.Map.

[chrisbobbe]

Thanks, LGTM! Some small things below; please merge at will after seeing those.

[chrisbobbe]

Hmm, should this style-guide entry—

Don't construct an Immutable.Map with an object-as-map: Whenever you
create a non-empty Immutable.Map, pass an array of entries instead of an
object-as-map. For example:

Immutable.Map([[HOME_NARROW_STR, [1, 2]]]) // good

Immutable.Map({ [HOME_NARROW_STR]: [1, 2] }) // BAD -- don't do

This is essential in order to get effective type-checking of the value
passed to the Immutable.Map function when a key is computed (like
HOME_NARROW_STR) instead of literal (like 'foo'). In the Immutable type
definition, the object-as-map branch involves an object type with an indexer
property, and in general Flow seems to have trouble with those.

—only forbid passing a literal object-as-map? If I assert that the object-as-map data has number values instead of UserPresence values, Flow does complain about that.

Oddly I don't get a Flow error with this:

diff --git src/presence/presenceModel.js src/presence/presenceModel.js
index a46559acb..9a3e8f2e3 100644
--- src/presence/presenceModel.js
+++ src/presence/presenceModel.js
@@ -225,7 +225,7 @@ export function reducer(
       //   mark it required.
       const data = action.data.presences ?? {};
       return {
-        byEmail: Immutable.Map(data),
+        byEmail: Immutable.Map<string, number>(data),
       };
     }

[gnprice]

Hmmm interesting. I'd forgotten that style-guide entry, thanks 😅

It seems like the type-checking is effective in this case. For example if I make the value type in the input data more general:

        byEmail: Immutable.Map((data: {| +[string]: mixed |})),

then I get an appropriate error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/presence/presenceModel.js:228:53

Cannot return object literal because mixed [1] is incompatible with
`UserPresence` [2] in type argument `V` [3] of property `byEmail`.
[incompatible-return]

   src/presence/presenceModel.js:228:53
    228│         byEmail: Immutable.Map((data: {| +[string]: mixed |})),
                                                             ^^^^^ [1]

References:
   src/reduxTypes.js:221:34
    221│   byEmail: Immutable.Map<string, UserPresence>,
                                          ^^^^^^^^^^^^ [2]
   node_modules/immutable/dist/immutable.js.flow:1056:23
   1056│ declare class Map<K, +V>
                               ^ [3]

Looking back at the chat thread we foresightedly linked to in that style-guide entry, and experimenting some more, I think the key thing that triggers that Flow issue is that it's not only an object literal but one with a computed property.

Compare:

      (Immutable.Map({ ['a' + 'b']: { older: true } }): Immutable.Map<string, UserPresence>);
      (Immutable.Map({ ab: { older: true } }): Immutable.Map<string, UserPresence>);

The first one gets no error, just like in that thread. But the first one correctly gets an error.

So I think we can narrow that style-guide proscription to apply only to computed property names — not to objects-as-maps in general, or even to object literals as maps.

[chrisbobbe]

nit, from style guide:

Always provide a type when writing an empty Immutable value:
Whenever you create an empty Immutable.Map, Immutable.List, or
so on, specify the intended type explicitly. For example:

const map: Immutable.Map<number, string> = Immutable.Map(); // good

const map = Immutable.Map(); // BAD -- don't do

This is essential in order to get effective type-checking of the
code that uses the new collection. (It's not clear if this is a bug
in Flow, or a design limitation of Flow, or an issue in the Flow types
provided by Immutable.js, or some combination.)

I think it could look like this:

diff --git src/presence/presenceModel.js src/presence/presenceModel.js
index a46559acb..a9f112efa 100644
--- src/presence/presenceModel.js
+++ src/presence/presenceModel.js
@@ -205,7 +205,7 @@ export const getPresenceStatusForUserId = (
 //
 
 const initialState: PresenceState = {
-  byEmail: Immutable.Map(),
+  byEmail: Immutable.Map<string, UserPresence>(),
 };

[gnprice]

Thanks, I'd forgotten to consider that.

I think this is covered, though, by the type annotation on the object as a whole — that : PresenceState. That's explicit in much the same way as the annotation in the "good" example.

And you can confirm it's effective by writing an expression like initialState.byEmail.values() and hovering in the IDE, to see it returns an iterator of UserPresence.

[chrisbobbe]

Hmm, I think the TODO is ripe for taking care of: we do enforce a threshold for ancient servers we refuse to connect to. (I noticed this when I saw the change from || to ?? and wondered if that might change any behavior, given our imperfect knowledge of ancient servers.)

[gnprice]

Hmm yeah, I guess that's right. I'll add a commit for that.