Distinguish types PmOutbox vs. StreamOutbox more fully#4998
Merged
chrisbobbe merged 4 commits intozulip:mainfrom Sep 11, 2021
Merged
Distinguish types PmOutbox vs. StreamOutbox more fully#4998chrisbobbe merged 4 commits intozulip:mainfrom
chrisbobbe merged 4 commits intozulip:mainfrom
Conversation
This expresses that the types of these properties `type`, `subject`, and `display_recipient` vary together, and that they do so in the same way as on `Message`. That in turn means that when we have a condition like `type === 'stream'`, Flow can infer that `display_recipient` will be a string (the stream name) rather than an array, and so on.
These two asserts just confirmed that the `type` and `display_recipient` properties varied together in the expected way, in order to assure Flow that after checking `type` we'd get the right type of value out of `display_recipient`. Now that we've expressed that tandem variation directly in the actual types, we can drop the asserts.
$Shape is not very sound -- Flow will sometimes treat the result as if
it definitely has all the original object type's properties.
Instead, use `$Rest<Foo, { ... }>` here, just as we do for
`streamMessage` and others in this file.
In fact, as a bonus simplification, fuse this use of `$Rest` with an
inexact object type (for making everything optional) with the existing
use of `$Diff` with an exact object type (for excluding a specific
property), resulting in a single `$Rest` with a combined type.
This is really an internal bit of how we go about constructing the types PmOutbox and StreamOutbox; it doesn't make much sense for types elsewhere in the application to refer to it. There was one place we'd been referring to it, at `outboxMessageBase` in `exampleData`, and that illustrates the point. Drop that type annotation entirely. Like `messagePropertiesBase`, this value is an internal helper used narrowly just within this module; its real type, in terms of the promise made in its interface, is "whatever the call site needs". Flow inherently checks that it satisfies *that* type. (Try it, after this change: delete one of the properties in the `outboxMessageBase` definition, and see an error appear where it's used in `streamOutbox`. This only works after the preceding commit which fixed `streamOutbox`'s use of the unsound `$Shape`.) And on the other hand, writing out what the type works out to explicitly is kind of verbose and just a recipe for confusion.
Contributor
|
Thanks, LGTM! Merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This expresses that the types of certain properties on
Outboxvary together, and that they do so in the same way as onMessage.That in turn means that when we have a condition like
type === 'stream', Flow can infer thatdisplay_recipientwill be a string (the stream name) rather than an array, and so on.Also make some followup simplifications that this opens up.
(
One upcoming place I suspect this will simplify the code a bit is at #4899 (comment)Hmm, no -- that code wantsstream_id, and that's something we don't yet have onOutbox: #3998. Ah well, this will simplify that code once we also have #3998.)