Remove redundant, stringly-typed fields from PmConversationData #4116

rk-for-zulip · 2020-05-18T03:51:53Z

Replace the excessively quirky ids: string and recipients: string fields in PmConversationData with a users: Users[] field, unconditionally containing all participants.

Partly in support of #3764 and #4035. Blocks PR #4104 (although see below).

... but also to simplify further refactoring.

rk-for-zulip · 2020-05-18T03:52:31Z

Somewhere around my 3.7th [sic] rebase-slash-rewrite of PR #4104, I got frustrated enough to do this first. This may not be the only way to keep the final version of that PR reasonable, but it's probably the best.

There is some overlap here with the commits in #4104: the commits for well-typing the relevant test code are taken from it. I believe I have addressed all of @gnprice's feedback for those commits in this version, with one notable exception (the NULL_OBJECT bit); I'll make a note of that as a "review" comment below. (In retrospect, I suppose these should have been split out into a separate PR.)

The final five commits (starting at recipient [nfc]: Generalize `filterRecipients`.) are somewhat speculative, and I shall be unsurprised if they provoke lengthy discussion. This is fine. It's certainly better that we merge the new logic in PmConversationList with the existing logic in recipient.js, but discussion on exactly how to go about doing that shouldn't block the preceding parts of the commit, and those are the parts that will unblock #4104.

src/__tests__/lib/exampleData.js

rk-for-zulip · 2020-05-18T03:57:49Z

src/__tests__/lib/exampleData.js

+export const makeMessagesState = (messages: Message[]): MessagesState =>
+  objectFromEntries(messages.map(m => [m.id, m]));


Feedback at #4104 (comment) resolved by a) stripping the exactness from both MessagesState itself (until Flow v0.111.0) and its indexer-property's value type, and b) eliminating the $FlowFixMe entirely.

gnprice

Thanks @ray-kraesig ! Happy to get these data structures cleaned up. Comments below about some of the specifics.

gnprice · 2020-05-19T00:14:20Z

src/types.js

+  /**
+   * A comma-separated sequence of the emails of the users involved in this
+   * conversation, sorted by user ID. Does not include the self-user, unless the
+   * self-user is the only recipient.
+   */
  recipients: string,


Is this true?

I don't think it is. See the implementation of normalizeRecipientsSansMe, and normalizeRecipients which it invokes. We sort there, by email address.

Whoops! Will fix.

gnprice · 2020-05-19T00:16:46Z

src/pm-conversations/PmConversationList.js

-  handleGroupNarrow = (email: string) => {
-    this.props.dispatch(doNarrow(groupNarrow(email.split(','))));
+  handleGroupNarrow = (users: UserOrBot[]) => {
+    this.props.dispatch(doNarrow(groupNarrow(users.map(s => s.email))));


The order here matters, because we use it for the key to look up the narrow in our data structures. So it's important to preserve the order used by the existing code, or else to carefully untangle to make it stop mattering.

Also, doesn't this now see the full set of users? Where previously it saw the set excluding self.

That's odd – I specifically remember seeing that issue and fixing it. (The second one, not the first; the first is a carryover of my mistake in describing recipients.) Will fix again.

gnprice · 2020-05-19T00:20:30Z

src/pm-conversations/PmConversationList.js

+  /**
+   * If this is a 1:1 conversation, the other user involved. (For the
+   * self-conversation, this is the self-user.)
+   *
+   * If this is a group PM, null.
+   */
+  getOtherUser = (users: UserOrBot[]): UserOrBot | null => {
+    const ownUserId = this.props.ownUser.user_id;
+    if (users.length <= 2) {
+      return users.filter(u => u.user_id !== ownUserId)[0];
+    }
+    return null;


Hmm, does this work in the self-1:1 case? It looks like it'd end up returning [][0], which is undefined.

gnprice · 2020-05-19T00:29:53Z

src/pm-conversations/PmConversationList.js

  handleGroupNarrow = (users: UserOrBot[]) => {
-    this.props.dispatch(doNarrow(groupNarrow(users.map(s => s.email))));
+    const { dispatch, ownUser } = this.props;
+    const emails = users.filter(u => u.user_id !== ownUser.user_id).map(u => u.email);
+    dispatch(doNarrow(groupNarrow(emails)));
+  };


Ah, I see. This change belongs in the earlier commit where we changed what set of users gets passed here.

– oh, there it is. I must have squashed the fix into the wrong commit. 😓

gnprice · 2020-05-19T00:36:25Z

src/unread/UnreadCards.js

    const unreadCards: Array<Card> = [
      {
        key: 'private',
-        data: [{ conversations, ...restProps }],
+        data: [conversations],


Hmm, can this really work with how this data gets used below?

section.key === 'private' ? ( <PmConversationList {...item} />

How does it know to call the prop conversations?

This can't possibly have worked. I think when I tested it last night I wasn't actually running the right version of the code. 😓

gnprice · 2020-05-19T01:16:09Z

src/reduxTypes.js

-|};
+// This type cannot be made exact until Flow v0.111.0.
+export type MessagesState = {
+  [id: number]: Message,


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.

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.)

gnprice · 2020-05-19T01:23:52Z

The final five commits (starting at recipient [nfc]: Generalize `filterRecipients`.) are somewhat speculative, and I shall be unsurprised if they provoke lengthy discussion. This is fine.

As I think you guessed, I am not a fan of these last few changes. 🙂 They make this code significantly more complex, and I don't see a corresponding gain.

... following a brief archaeological expedition.

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.

Additionally, make their arguments exact, to raise errors on unrecognized arguments.

... an expansion of `pmMessage` allowing (and requiring) the caller to supply the users involved in the message.

... which assembles a `MessagesState` from the `Message`s that compose it.

Translate existing tests from JavaScript to Flow. (There is no significant functional difference, but there are some minor changes to irrelevant data under the hood.)

PmConversationData has two distinct and terrible ways of passing down the list of users. Add a third way (presumably less terrible), which will replace both of them.

... instead of threading the relevant Redux state through its callers. (Consequently, eliminate the now-unused additional plumbing.)

Add a new function to `recipient.js` which will substitute for `normalizeRecipientsSansMe` in the private conversation UI. The new function is no longer stringly-typed, but is not otherwise an improvement. (Also, `normalize` is a terrible name for what any of these do.)

Remove its dependence on the old stringly-typed fields. The existing code has relied on the selector to munge the data for UI presentation; this now becomes the responsibility of the UI component itself. (The logic to do so is presently inline, but is expected to be unified with existing related logic in `recipient.js` later.)

The key used here doesn't have to be anything in particular, so long as it's consistent. We assemble a nonce-key not used elsewhere. (Or, if it does happen to be used elsewhere, we don't care.)

For the purposes of eliminating all but the latest message per conversation, one uniquely-identifying string-key is as good as another. Since we no longer need to send `recipients` downstream, we might as well just use `ids` -- in which case there's no longer any need for `normalizeRecipientsSansMe` here.

... and, following the previous commit, there are no more uses of `normalizeRecipientsSansMe`. Eliminate it. Convert its tests to be used by the function which has effectively replaced it, instead.

rk-for-zulip · 2020-05-20T22:28:36Z

The new version may be politely described as less ambitious. It may be less politely described as "just pushing the junk from one room into another room".

The semantic mess that is normalizeRecipientsSansMe is no longer in a place where it gets in the way of work on #3133, but it hasn't been eliminated – just moved, with a new coat of paint on it.

(But at least we're no longer passing two different stringly-typed values in.)

gnprice · 2020-12-25T03:33:13Z

The main goals of this PR have been taken care of separately since then, with the PR series up to #4356.

I've just sent #4358 with (rebased versions of) the parts of this PR that I think are useful and still relevant, plus some related new changes. Closing this one.

selectors [nfc]: Rename some variables, mostly for clarity.

b709601

... but also to simplify further refactoring.

rk-for-zulip requested a review from gnprice May 18, 2020 03:51

rk-for-zulip commented May 18, 2020

View reviewed changes

src/__tests__/lib/exampleData.js Show resolved Hide resolved

rk-for-zulip commented May 18, 2020

View reviewed changes

gnprice mentioned this pull request May 19, 2020

Use recent_private_conversations on PMs screen, when available #4104

Closed

gnprice reviewed May 19, 2020

View reviewed changes

rk-for-zulip added 15 commits May 20, 2020 10:38

narrow [nfc]: Add some documentation for groupNarrow.

d3da775

... following a brief archaeological expedition.

example data [nfc]: Allow specifying user_id when making a user.

288b360

Additionally, make their arguments exact, to raise errors on unrecognized arguments.

example data: Add auxiliary function pmMessageFromTo.

f82b9ff

... an expansion of `pmMessage` allowing (and requiring) the caller to supply the users involved in the message.

example data: Add auxiliary function makeMessagesState.

1e6668f

... which assembles a `MessagesState` from the `Message`s that compose it.

tests: Properly type pmConversationsSelectors-test.js.

b80309a

Translate existing tests from JavaScript to Flow. (There is no significant functional difference, but there are some minor changes to irrelevant data under the hood.)

types [nfc]: Add jsdoc for the fields of PmConversationData.

1377fa2

recent convs: Add users field to PmConversationData.

75bf388

PmConversationData has two distinct and terrible ways of passing down the list of users. Add a third way (presumably less terrible), which will replace both of them.

pm UI [nfc]: Connect PmConversationList to Redux directly.

fd5d315

... instead of threading the relevant Redux state through its callers. (Consequently, eliminate the now-unused additional plumbing.)

pm UI: Finish converting PmConversationList to use users.

3cd885b

The key used here doesn't have to be anything in particular, so long as it's consistent. We assemble a nonce-key not used elsewhere. (Or, if it does happen to be used elsewhere, we don't care.)

types [nfc]: Remove ids and recipients from PmConversationData.

c3c9660

recipient.js: Remove normalizeRecipientsSansMe.

f371f09

... and, following the previous commit, there are no more uses of `normalizeRecipientsSansMe`. Eliminate it. Convert its tests to be used by the function which has effectively replaced it, instead.

rk-for-zulip force-pushed the downstream-nonsense-first branch from 2758052 to f371f09 Compare May 20, 2020 21:38

gnprice mentioned this pull request Dec 25, 2020

Refresh the still-useful parts of #4116, on path to using recent-PMs data #4358

Merged

gnprice closed this Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant, stringly-typed fields from PmConversationData #4116

Remove redundant, stringly-typed fields from PmConversationData #4116

rk-for-zulip commented May 18, 2020

rk-for-zulip commented May 18, 2020 •

edited

Loading

rk-for-zulip May 18, 2020

gnprice left a comment

gnprice May 19, 2020

rk-for-zulip May 19, 2020

gnprice May 19, 2020

gnprice May 19, 2020

rk-for-zulip May 19, 2020

gnprice May 19, 2020

gnprice May 19, 2020

rk-for-zulip May 19, 2020

gnprice May 19, 2020

rk-for-zulip May 19, 2020

gnprice May 19, 2020

rk-for-zulip May 19, 2020

gnprice commented May 19, 2020

rk-for-zulip commented May 20, 2020 •

edited

Loading

gnprice commented Dec 25, 2020

		export const makeMessagesState = (messages: Message[]): MessagesState =>
		objectFromEntries(messages.map(m => [m.id, m]));

Remove redundant, stringly-typed fields from PmConversationData #4116

Remove redundant, stringly-typed fields from PmConversationData #4116

Conversation

rk-for-zulip commented May 18, 2020

rk-for-zulip commented May 18, 2020 • edited Loading

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented May 19, 2020

rk-for-zulip commented May 20, 2020 • edited Loading

gnprice commented Dec 25, 2020

rk-for-zulip commented May 18, 2020 •

edited

Loading

rk-for-zulip commented May 20, 2020 •

edited

Loading