Skip to content

Commit

Permalink
user status types: Say that /register response data is string-keyed
Browse files Browse the repository at this point in the history
All JS objects are string-keyed; see Greg's explanation at
  zulip#5224 (comment)

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
  • Loading branch information
chrisbobbe committed Feb 23, 2022
1 parent ddb736a commit 9d15e07
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 7 deletions.
9 changes: 3 additions & 6 deletions src/api/initialDataTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -511,12 +511,9 @@ export type InitialDataUserStatus = $ReadOnly<{|
*/
// TODO(server-1.9.1): Make required.
user_status?: $ReadOnly<{|
// TODO(flow): The key here is really UserId, not just any number; but
// this Flow bug:
// https://github.com/facebook/flow/issues/5407
// means that doesn't work right, and the best workaround is to
// leave it as `number`.
[userId: number]: $ReadOnly<{|
// Keys are UserId encoded as strings (just because JS objects are
// string-keyed).
[userId: string]: $ReadOnly<{|
// TODO: add status emoji properties
// TODO: Comment on what these mean (if doc not fixed):
// https://chat.zulip.org/#narrow/stream/412-api-documentation/topic/Emoji.20statuses.20in.20zulip.2Eyaml/near/1322329
Expand Down
17 changes: 16 additions & 1 deletion src/user-status/userStatusReducer.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
/* @flow strict-local */

import { objectFromEntries } from '../jsBackport';
import { makeUserId } from '../api/idTypes';
import objectEntries from '../utils/objectEntries';
import type { UserStatusState, PerAccountApplicableAction } from '../types';
import {
LOGOUT,
Expand All @@ -22,7 +26,18 @@ export default (
return initialState;

case REGISTER_COMPLETE:
return action.data.user_status || initialState;
return objectFromEntries(
objectEntries(action.data.user_status ?? {}).map(([id, status]) => [
// Converting from string keys to numeric ones here doesn't
// actually make a difference to how the state is represented
// at runtime. But it will when we start using
// Immutable.Map<UserId, UserStatus> soon, and it satisfies
// Flow.
// TODO: Use Immutable.Map<UserId, UserStatus>
makeUserId(Number.parseInt(id, 10)),
status,
]),
);

case EVENT_USER_STATUS_UPDATE: {
const newUserStatus = { ...state[action.user_id] };
Expand Down

0 comments on commit 9d15e07

Please sign in to comment.