Outbox type: Make sender_id required; add optional stream_id.#4667
Outbox type: Make sender_id required; add optional stream_id.#4667chrisbobbe merged 9 commits intozulip:masterfrom
sender_id required; add optional stream_id.#4667Conversation
WesleyAC
left a comment
WesleyAC
left a comment
There was a problem hiding this comment.
LGTM, except for either tracking or actually fixing the exponential-spread warning.
Thanks for doing this! Having the stream_id in Outbox I think will let us get rid of stream_name in the actionsheet code, which I'm excited about :)
| const outputTimestamp = timestamp ?? makeTime() / 1000; | ||
| return deepFreeze({ | ||
| ...outboxMessageBase, | ||
| /* $FlowFixMe[exponential-spread] - Yeah, we should probably deal |
There was a problem hiding this comment.
Can you make a ticket to track this, or do it in this PR?
I guess it's not a huge deal, since this is just a problem with typechecking performance, not runtime performance, but it does seem like something we should fix.
chrisbobbe
force-pushed
the
pr-outbox-sender-stream-ids
branch
2 times, most recently
from
c2cd382 to
9aeef9a
Compare
|
Thanks for the review! Revision pushed. For the Flow error, it seemed sufficient to tweak the type of |
|
This looks good to me, feel free to merge after you fix the conflict :) |
This provides some helpful context; it also this helps make it clear that (in 3ab6a51) we were adding a `browser` property that didn't exist before, rather than changing the value of an existing `browser` property.
As v27.157 has been out for well over a few weeks. And write a migration to strip any outbox messages that don't have it, as noted we should in a1fad7c's TODOs and in zulip#3998.
Now that we've made it required, as it has been on `Message`. Also remove a heading that we probably wished we could have removed in 34bf18b; the `SubsetProperties` of `Message` makes the meaning quite clear.
Only `RenderedMessageDescriptor`, not `RenderedTimeDescriptor`, has an `.isBrief` property. As written, it looks like these tests already fail when `.isBrief` is missing (the `expect` would receive `undefined` instead of a boolean), so this change just makes the expectation more explicit, and will let this code pass type-checking, which we'll start doing soon.
From a search of `sender_email` in all JS files, I didn't see any other obvious places we could take advantage of `sender_id`. Part of zulip#3998.
Like we did with `PmMessage` and `StreamMessage` in zulip#4506. We don't actually make `PmOutbox` any different from `StreamOutbox` in this commit; that'll come later.
…`es. To reinforce the point in the section header: """ (Only stream messages for now. Feel free to add PMs, if you need them.) """ We don't need to handle PMs yet, but this'll hopefully make it clearer how to do so when the time comes.
Much like we did for `sender_id` in a1fad7c. Once a release that has this commit has been out for a few weeks, we should write a migration to strip `StreamOutbox` messages that don't have `stream_id`, and make `stream_id` required. Part of zulip#3998.
chrisbobbe
force-pushed
the
pr-outbox-sender-stream-ids
branch
from
9aeef9a to
fce3e8b
Compare
Thanks for the review! Done; I also added a commit at the beginning to put a quick comment on migration 28. |
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe and @WesleyAC ! Glad to have this taken care of.
I've just pushed the following commits following up on these changes:
6fe63e3 outbox types: Mark PmOutbox and StreamOutbox as read-only.
8171f2d outbox types [nfc]: Fix TODO comment on adding stream_id.
606591b outbox types [nfc]: Add TODO comments on PmOutbox vs. StreamOutbox.
One is trivial. The others are comment-only changes, corresponding to the comments below -- take a look.
I've also drafted up a fix for one of these, completing the distinction between PmOutbox and StreamOutbox. Branch here:
master...gnprice:dev-outbox-types
but it needs some cleaning-up before merge, and it's late here now and I should go to bed 🙂
| ...SubsetProperties< | ||
| // Could use `MessageBase` here, but Flow would complain anyway if | ||
| // we tried to put something that's not in `MessageBase` below. | ||
| Message, |
There was a problem hiding this comment.
Hmm, this doesn't seem to be true 🙂 In fact, several of these aren't on MessageBase -- they're just on both PmMessage and StreamMessage, separately. If I change this to say MessageBase, Flow gives errors here about those as one would hope:
Cannot instantiate SubsetProperties because property display_recipient is missing
in MessageBase [1] in the first argument. [prop-missing]
…
Cannot instantiate SubsetProperties because property subject is missing in
MessageBase [1] in the first argument. [prop-missing]
…
Cannot instantiate SubsetProperties because property type is missing in
MessageBase [1] in the first argument. [prop-missing]
The consequence of putting them here is that they each get broken down into unions separately: so type: 'stream' | 'private' and display_recipient: string | $ReadOnlyArray<PmRecipientUser>, but losing the information that a type of 'stream' means a display_recipient that's a string, and so on.
Of course that's the way it already was before this change, when we didn't have PmOutbox and StreamOutbox at all. So that's why no existing code gives a type error because of this loss of information. But it means that if e.g. streamNameOfStreamMessage were to try to rely on the PmOutbox / StreamOutbox distinction -- by dropping its invariant call where it checks that display_recipient is indeed a string -- it would get a type error, because that distinction isn't yet functioning.
| // TODO(#3764): Make stream_id required. Needs a migration to drop | ||
| // StreamOutbox values that lack it; that'll be fine once the | ||
| // release that adds it has been out for a few weeks. (Also drop |
There was a problem hiding this comment.
There's another key step missing before we can do this: we need to start actually populating this property in the values we create for state.outbox 😉 See a1fad7c for comparison.
|
Thanks for catching those things, @gnprice! Please let me know if you'd like me to take over master...gnprice:dev-outbox-types; otherwise I'll take a look when you make it into a PR. 🙂 |
Compare a1fad7c, which did the same for the sender_id property. We actually did part of this a few months ago, in fce3e8b in zulip#4667. That commit added it to the type, but we hadn't yet taken the step of supplying this property in any actual Outbox values. Also add back to a TODO comment a bit that got dropped in 8171f2d.
Compare a1fad7c, which did the same for the sender_id property. We actually did part of this a few months ago, in fce3e8b in zulip#4667. That commit added it to the type, but we hadn't yet taken the step of supplying this property in any actual Outbox values. Also add back to a TODO comment a bit that got dropped in 8171f2d.
Compare a1fad7c, which did the same for the sender_id property. We actually did part of this a few months ago, in fce3e8b in zulip#4667. That commit added it to the type, but we hadn't yet taken the step of supplying this property in any actual Outbox values. Also add back to a TODO comment a bit that got dropped in 8171f2d.
Compare a1fad7c, which did the same for the sender_id property. We actually did part of this a few months ago, in fce3e8b in zulip#4667. That commit added it to the type, but we hadn't yet taken the step of supplying this property in any actual Outbox values. Also add back to a TODO comment a bit that got dropped in 8171f2d. This makes zulip#3998 mostly complete: the remaining steps will be to get a release with this change out, wait a few weeks, then make `stream_id` required with a migration to drop `Outbox` values that lack it, just as we did for `sender_id` before. Issue: zulip#3998
Compare a1fad7c, which did the same for the sender_id property. We actually did part of this a few months ago, in fce3e8b in zulip#4667. That commit added it to the type, but we hadn't yet taken the step of supplying this property in any actual Outbox values. Also add back to a TODO comment a bit that got dropped in 8171f2d. This makes zulip#3998 mostly complete: the remaining steps will be to get a release with this change out, wait a few weeks, then make `stream_id` required with a migration to drop `Outbox` values that lack it, just as we did for `sender_id` before. Issue: zulip#3998
This property is required (since 60847c9 / #4667, earlier this year), so we can rely on it to simplify away this workaround. This workaround was in fact the original prompt that caused me to notice we didn't have sender_id (or stream_id) on Outbox, and file #3968 (comment)
We started supplying this property in 0c251bf / #5000. That went out to users a few weeks ago, in v17.171 which rolled out 2021-09-23, so by this point the bulk of our users have upgraded to that version. Now we mark the property as required, and drop any old outbox messages that lack it in the unlikely case that there are any. This completes #3998 because we've already done the same thing for `sender_id`, in a1fad7c / #4304 and then 60847c9 / #4667. Fixes: #3998
This property is required (since 60847c9 / zulip#4667, earlier this year), so we can rely on it to simplify away this workaround. This workaround was in fact the original prompt that caused me to notice we didn't have sender_id (or stream_id) on Outbox, and file zulip#3968 (comment)
We started supplying this property in 0c251bf / zulip#5000. That went out to users a few weeks ago, in v17.171 which rolled out 2021-09-23, so by this point the bulk of our users have upgraded to that version. Now we mark the property as required, and drop any old outbox messages that lack it in the unlikely case that there are any. This completes zulip#3998 because we've already done the same thing for `sender_id`, in a1fad7c / zulip#4304 and then 60847c9 / zulip#4667. Fixes: zulip#3998
3 participants
Finish most of #3998! 🎉
sender_idrequired, since the change that added it as optional has been out in a release for a whilestream_id, as optional, so the process starts again: once that's been out for a few weeks, we should make that required, and that'll finish off Add sender_id and stream_id properties to Outbox #3998.Also start type-checking another test file.