Introduce invariant; use it to handle display_recipient more cleanly.
#4329
Conversation
The immediate motivation for this is that I'd like to put a real type
on `Message#display_recipient` -- currently it's $FlowFixMe -- and
that means that the entries here which are just `{ email: string }`
will have to go.
While we're here, we take the opportunity to do some related
cleanups, some of which help us make this change in the first place:
* Stop explicitly providing `display_recipient` for messages; instead,
use `recipients` for PMs or `stream` for stream messages. This
simplifies the code for PMs and gets us stream IDs for stream messages.
* Use more named example values, and less of string constants like
`'some stream'`.
* Cut some spots where we were passing values that are redundant with
defaults or irrelevant to the test case.
These arrays and properties are certainly meant to be treated as read-only; always good to make that explicit where applicable. This will also let us add some read-only types upstream of here.
See docs in diff for discussion.
I.e., the invariant that display_recipient means one thing when message.type is 'stream', and another when 'private'. With this change we also provide more-descriptive names in many of these spots, like `streamName` for the name of a stream. After this change and the series that follows it, we'll (a) never consume `display_recipient` without making explicit whether we think we're looking at a PM or a stream message, and (b) have a real type on it so that if we did ever consume it directly, the type-checker can remind us to explicitly confront the wrinkly type it has.
Now the type-checker can confirm that we explicitly handle the two possibilities here. The previous commit set us up so that this passes Flow, mostly by centralizing accesses to display_recipient through a pair of helpers that explicitly confirm the type is the one we expect. There are still a number of situations where if we're sloppy about this the type-checker won't catch it: in particular if we pass the display_recipient to `===` or `JSON.stringify`, which accept anything, or get its `.length` which exists on both strings and arrays. We'll clean those up too in upcoming commits.
src/types.js
Outdated
| @@ -2,6 +2,7 @@ | |||
| import type { IntlShape } from 'react-intl'; | |||
| import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet'; | |||
|
|
|||
| import type { SubsetProperties } from './generics'; | |||
| import type { Auth, Topic, Message, PmRecipientUser, Reaction, ReactionType } from './api/apiTypes'; | |||
There was a problem hiding this comment.
d220cc8 types: Use SubsetProperties to base Outbox more directly on Message.
tools/test lint tells me Reaction and PmRecipientUser are defined but never used, in this commit.
There was a problem hiding this comment.
tools/test linttells meReactionandPmRecipientUserare defined but never used, in this commit.
Ah, thanks for the catch! Fixed.
| display_recipient: string | NamedUser[], | ||
| subject: string, | ||
| |}; | ||
| type DataFromNarrow = SubsetProperties< |
There was a problem hiding this comment.
In fact, use our SubsetProperties type-helper to get
the real thing directly from the actual Outbox type so we don't
have to duplicate the definition.
Neat!
| @@ -322,7 +329,8 @@ describe('getNarrowFromMessage', () => { | |||
| expect(actualNarrow).toEqual(expectedNarrow); | |||
| }); | |||
|
|
|||
| test('if recipient of a message is string, returns a stream narrow', () => { | |||
| test('for stream message with empty topic, returns a stream narrow', () => { | |||
| // TODO this behavior seems pretty dubious | |||
|
This looks great! I've made just a few comments, all small. It's so nice to be able to clean up |
| if (action.type === 'MESSAGE_FETCH_START') { | ||
| expect(action.numBefore).not.toBeGreaterThan(0); | ||
| } | ||
| invariant(action.type === 'MESSAGE_FETCH_START', 'expect failed'); |
There was a problem hiding this comment.
Nice! Yeah, I felt weird adding these conditionals. But less weird than adding $FlowFixMes. Glad to have this solution.
This actually turned up one small discrepancy! The `reactions` property's type was plain `Reaction[]` here, but over on Message it's `$ReadOnlyArray<Reaction>`. While we're closely comparing these, also add a `$ReadOnly` on the whole type to match the one on `Message`.
This was really just an approximation of PmRecipientUser; use the real thing. In fact, use our SubsetProperties type-helper to get the real thing directly from the actual Outbox type so we don't have to duplicate the definition.
These recipient lists never happen in real life -- display_recipient on a PM always contains *all* users involved, including self. As a result, we weren't testing the PM scenarios that actually happen in real life. While here, also exercise the self-1:1 case, as that's a common source of bugs in this kind of logic.
In addition to helping along our move toward stable numeric IDs everywhere, this aligns this function's logic with relevant helpers in recipient.js, which sets us up to have it just call one of them.
That union was indeed confusing... and it turned out it was serving as basically a roundabout $FlowFixMe for one of this function's callers! Fix that type issue directly, and simplify. All callers do indeed always pass an actual array -- and the behavior would be broken or meaningless anyway if they didn't.
…here. This follows up on the commit earlier in this series where we added these helpers `streamNameOfStreamMessage` and `recipientsOfPrivateMessage` and started using them everywhere that Flow could detect we were implicitly relying on an assumption about which kind of display_recipient we had. In this commit, we cover everywhere else. Now we never mention this property at all, except in the type itself; where we construct values with it; and in those two encapsulating helpers in recipient.js. Well, and in comments. :-)
This expresses more directly what we really mean here -- the conditionals were only for Flow's sake and we didn't really mean them as conditionals, because we believe the `else` case can never happen.
gnprice
force-pushed
the
pr-invariant-recipient
branch
from
313ebbe to
ae7cbed
Compare
|
Merged, with that one fix. Thanks for the review!
Yeah. It did stall on that when I picked it up in... February, IIRC. I then spent a solid few days studying the I'm not sure offhand how the bits that have landed compare to what was blocking #3133, or what might still be needed for that; I'd have to go take a look to page that back into my head. |
This starts us using Flow's support for an assertion function named
invariant, which I learned about recently: facebook/flow#6052The immediate inspiration is the last commit in this series, a small test cleanup which clears things up for #4327. I thought a bit about more substantial ways to make use of it, and landed on a refactoring of how we handle the notorious
display_recipientproperty on messages: at the end of this branch, when we consumedisplay_recipientwe always do so through a pair of helpers inrecipient.js, which explicitly check our assumptions and return well-typed results.There's also a few other cleanups in here which either came up as prerequisites for the main changes, or were suggested by them and quick to add in.