api: Sync update-stream event types with doc and align implementation #5346
Conversation
Well, sort of.
There are some good changes in behavior: - When the event's `property` is "invite_only", we'll also update `event.history_public_to_subscribers` and `is_web_public`. This fixes a live bug! We read `is_web_public` when choosing whether to mark a stream as web-public in the UI. - When the event's `property` is "description", we'll also update `rendered_description`. (This was a latent bug; we don't read `rendered_description` anywhere.) So, also add tests for those.
| expect( | ||
| streamsReducer( | ||
| [], | ||
| deepFreeze({ |
There was a problem hiding this comment.
streamsReducer tests [nfc]: Make more compact
Well, sort of.
Yeah, Prettier's style of layout makes for a lot of very short lines here.
One thing I sometimes do when that happens is to pull some pieces out as locals, in order to get Prettier to collapse a list of arguments back to one line. Here that could look like:
- expect(
- streamsReducer(
- [],
- deepFreeze({
- type: EVENT,
- event: {
- id: 0,
- type: EventTypes.stream,
- op: 'create',
- streams: [stream1, stream2],
- },
- }),
- ),
- ).toEqual([stream1, stream2]);
+ const event = deepFreeze({
+ type: EVENT,
+ event: {
+ id: 0,
+ type: EventTypes.stream,
+ op: 'create',
+ streams: [stream1, stream2],
+ },
+ });
+ expect(streamsReducer([], event)).toEqual([stream1, stream2]);There was a problem hiding this comment.
A further step could be, at the top of this file:
const mkAction = e => deepFreeze({ type: EVENT, event: { id: 0, type: EventTypes.stream, ...e } });and then these 14 lines would become just:
const action = mkAction({ op: 'create', streams: [stream1, stream2] });
expect(streamsReducer([], action)).toEqual([stream1, stream2]);
and similarly for the other events in the file.
There was a problem hiding this comment.
A further step
I've just tried this out, and I haven't yet found a way to do this without Flow errors; I think a factor is how many branches there are in StreamEvent.
There was a problem hiding this comment.
Maybe I'll come back to it in the future when doing other substantive work in this file.
There was a problem hiding this comment.
Just sent #5352 for the first thing though.
There was a problem hiding this comment.
I've just tried this out, and I haven't yet found a way to do this without Flow errors; I think a factor is how many branches there are in
StreamEvent.
Ah indeed. I think I have a solution for that; I'll send as a PR after merging #5352.
There was a problem hiding this comment.
OK, sent: #5353. The extra bit it needs is a type annotation on the helper, making it generic with a bound:
const mkAction = <E: $Diff<StreamEvent, {| id: mixed, type: mixed |}>>(event: E) =>
deepFreeze({ type: EVENT, event: { id: 0, type: EventTypes.stream, ...event } });Wrote a bit of a long commit message for it, too -- I tried to think through why it is that this was getting errors without an annotation, and why this particular spot is one where Flow needs an annotation even though in many other situations it infers things just fine, and then to see if I could explain that in a meaningful way.
| | {| | ||
| ...StreamUpdateEventBase, | ||
| +property: 'rendered_description', | ||
| +value: $PropertyType<Stream, 'rendered_description'>, | ||
| |} | ||
| | {| | ||
| ...StreamUpdateEventBase, | ||
| +property: 'is_web_public', | ||
| +value: $PropertyType<Stream, 'is_web_public'>, | ||
| |} |
There was a problem hiding this comment.
I think these can usefully be further deduplicated. I'll send that as a quick PR on top of this.
After #5341, on the path to fixing #5250.