Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant, stringly-typed fields from PmConversationData #4116

Closed
wants to merge 16 commits into from
+210 −241
Closed

Remove redundant, stringly-typed fields from PmConversationData #4116

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b709601
selectors [nfc]: Rename some variables, mostly for clarity.
rk-for-zulip May 9, 2020
d3da775
narrow [nfc]: Add some documentation for `groupNarrow`.
rk-for-zulip May 18, 2020
b19dc56
types: Remove all exactness from MessagesState.
rk-for-zulip May 14, 2020
288b360
example data [nfc]: Allow specifying `user_id` when making a user.
rk-for-zulip May 11, 2020
f82b9ff
example data: Add auxiliary function `pmMessageFromTo`.
rk-for-zulip May 11, 2020
1e6668f
example data: Add auxiliary function `makeMessagesState`.
rk-for-zulip May 11, 2020
b80309a
tests: Properly type pmConversationsSelectors-test.js.
rk-for-zulip May 11, 2020
1377fa2
types [nfc]: Add jsdoc for the fields of PmConversationData.
rk-for-zulip May 15, 2020
75bf388
recent convs: Add `users` field to `PmConversationData`.
rk-for-zulip May 17, 2020
fd5d315
pm UI [nfc]: Connect `PmConversationList` to Redux directly.
rk-for-zulip May 19, 2020
f0311ce
recipient.js: Add replacement helper function.
rk-for-zulip May 20, 2020
7cc45cd
pm UI: Convert `GroupPmConversationItem` to use `users`.
rk-for-zulip May 17, 2020
3cd885b
pm UI: Finish converting `PmConversationList` to use `users`.
rk-for-zulip May 17, 2020
c3c9660
types [nfc]: Remove `ids` and `recipients` from `PmConversationData`.
rk-for-zulip May 18, 2020
6b52ada
recent convs: Remove `recipients` from internal structure.
rk-for-zulip May 18, 2020
f371f09
recipient.js: Remove `normalizeRecipientsSansMe`.
rk-for-zulip May 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
types: Remove all exactness from MessagesState. 
MessagesState is exact; until Flow v0.111.0, this is not usefully
compatible with indexer properties. This exactness was added in
8370002, as part of a mechanical
transformation, before we were aware of this issue.

MessagesState also has `$Exact<Message>` as its indexer value type,
rather than simply `Message`. This is the only use of `$Exact<...>`
directly as the type of a value in our codebase; it was added in
commit 01003e6, a professedly
experimental commit, where it is the only such change among several
less interesting `{ ... }` → `{| ... |}` conversions.

Revert both. Add a note about the former and Flow v0.111.0, for
grep's sake.
@rk-for-zulip
rk-for-zulip committed May 20, 2020
commit b19dc561a9a273b6d54ffddc82738f1279ea54ce
7 changes: 4 additions & 3 deletions src/reduxTypes.js
View file
Original file line number Diff line number Diff line change
@@ -153,9 +153,10 @@ export type FlagName = $Keys<FlagsState>;
* See also `NarrowsState`, which is an index on this data that identifies
* messages belonging to a given narrow.
*/
export type MessagesState = {|
[id: number]: $Exact<Message>,
|};
// This type cannot be made exact until Flow v0.111.0.
export type MessagesState = {
[id: number]: Message,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing exactness here, why not add it on Message itself? (And remove the then-redundant mention of it here.)

Otherwise we're regressing on how tightly we type-check.

For a quick experiment I made Message exact. Flow reported only one error -- and in fact it looks like a real one, which would have been caught in 3dc2d8f if we'd had Message exact then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message really isn't exact, though: it's full of unlisted fields from (possibly future versions of) the server. We shouldn't make the claim that it is. (The bug you cite is a good argument that eg.streamMessage and friends should take $Exact<Message>, though.)

I assume what we really want to do is to deconstruct the inexact, server-supplied ApiMessage object (to coin a name) into a fresh LocalMessage object that really is exact. But not, I think, today.

(I'm pretty sure this has come up before. I know I've documented it in Message at some point, but I suppose that hasn't made it into a PR yet. I'll try to dig it up and submit it separately. Or just rewrite it.)

};

export type MigrationsState = {|
version?: string,