narrow: Encapsulate the Narrow type within its module, checked with an opaque type#4342
narrow: Encapsulate the Narrow type within its module, checked with an opaque type#4342chrisbobbe merged 15 commits intozulip:masterfrom
Conversation
gnprice
force-pushed
the
pr-narrow-opaque
branch
from
1fde103 to
c0e19ba
Compare
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks, @gnprice! See a few comments, below.
| export const emailOfPm1to1Narrow = (narrow: Narrow): string => | ||
| caseNarrowPartial(narrow, { | ||
| pm: emails => { | ||
| invariant(emails.length === 1, 'emailOfPm1to1Narrow: got 1:1 narrow'); |
There was a problem hiding this comment.
Is that quite the right text for the second argument, the error message when the invariant is violated? IIUC, "got 1:1 narrow" describes the correct thing happening for a 1 to 1 PM narrow, not the wrong thing happening.
There was a problem hiding this comment.
Ah thanks! Indeed -- will fix.
| * into a data structure; | ||
| * * `caseNarrow` and its relatives, for pattern-matching or destructuring; | ||
| * * `ApiNarrow` for the form we put a narrow in when talking to the | ||
| * server, and `apiNarrowOfNarrow` for converting to it. |
There was a problem hiding this comment.
and
apiNarrowOfNarrowfor converting to it.
Nit: apiNarrowOfNarrow doesn't appear until near the end of this series.
| }); | ||
|
|
||
| /** | ||
| * The "PM key recipients" for a PM narrow; else error. |
There was a problem hiding this comment.
The "PM key recipients" for a PM narrow; else error.
Hmm, the part in quotes makes me think of the new PmKeyRecipients type in recipient.js, so much that I might expect it to be the return type, but it's not.
There was a problem hiding this comment.
Yeah -- the intention is to refer to that, but it's not the same type because it only has the emails.
This isn't very clear, so I'll add something to the jsdoc to clarify.
|
|
||
| render() { | ||
| const { narrow, stream, color } = this.props; | ||
| const topic = topicOfNarrow(narrow); |
There was a problem hiding this comment.
I think the narrow isn't known to be a topic narrow at this point; it might be a stream narrow, and topicOfNarrow would throw.
...Ah, and confirmed in manual testing. 🙂
There was a problem hiding this comment.
Indeed -- thanks for the catch! Will fix.
| * instead of a Narrow in the first place; or if they do handle other kinds | ||
| * of narrows, should be using `caseNarrow`. | ||
| */ | ||
| export const streamNameOfNarrow = (narrow: Narrow): string => |
There was a problem hiding this comment.
Nit: What's the pattern for how this and its neighbors are ordered? 🙂 I see three is*Narrows (starting from isHomeNarrow), then these *Of*Narrows...then seven more is*Narrows.
I could see all the is*Narrows being together in one section, and all the *Of*Narrows being together in another section.
Or I could see the related ones being paired/grouped together. For example, emailsOfGroupPmNarrow and emailsOfPmNarrow are about PMs, so they could sit alongside isGroupPmNarrow and is1to1PmNarrow.
There was a problem hiding this comment.
Yeah. It's gotten a bit chaotic, in part because I've sometimes alternated between those two ways of organizing it as I've added things.
Also in part because I've considered several different ideas over time for what the eventual interface of this module and the Narrow type should be, after all this refactoring, and the bug-fixing it brings out, is complete. Some of these getters (like emailsOfGroupPmNarrow) get deleted already in later parts of my draft narrow-refactoring branch. My current thinking is that once Narrow has a reasonable structure, these getters (*Of*Narrow) and predicates (is*Narrow) will all simplify into trivial property accesses, and we can make the type a normal non-opaque type alias again (or perhaps better: an opaque type with a bound equal to its real underlying type, like PmKeyRecipients, so that outside code still can't create values of the type) and just inline all those getters and predicates at their call sites, and delete them.
So I'm inclined to defer any major reshuffling of the order of them until late in this series of PRs, when we have a firmer idea of what the set of things in the interface ultimately is.
There was a problem hiding this comment.
My current thinking is that once
Narrowhas a reasonable structure, these getters (*Of*Narrow) and predicates (is*Narrow) will all simplify into trivial property accesses
That's amazing! 🤯
I wondered if something like that was on the horizon; I'm glad it is.
So I'm inclined to defer any major reshuffling of the order of them until late in this series of PRs, when we have a firmer idea of what the set of things in the interface ultimately is.
Cool, sounds good.
gnprice
force-pushed
the
pr-narrow-opaque
branch
from
c0e19ba to
194a0ae
Compare
|
New revision pushed! |
We've been taking the data structure that's used in a couple of spots in the Zulip API as our internal representation of a narrow throughout the app. It's a pretty bad fit for that purpose, and we're going to change to a more reasonable one. Lay the groundwork by marking the distinction between the handful of places where we actually mean the API-facing narrow type, and the rest where we want our own internal narrow type. For the present this is just a type alias, because lots of places in our code still intimately depend on the details of this representation. In the upcoming series of commits, we'll fix those so that they all use explicit interfaces provided by the `narrow` module, after which we can switch to an opaque type alias so that Flow confirms that nothing outside that module still depends on the specifics of the type. That will then set us up for future changes that actually alter the type.
We're going to change our internal representation of narrows soon to something more reasonable. The internal representation doesn't belong in tests anyway, so remove it.
We're going to change the internal representation of narrows soon. The internal representation isn't really any test's business in the first place, and it especially was never the business of any of these tests that aren't of the `narrow` module itself. Instead, they should use the proper constructors and constants from the `narrow` module.
Instead of treating these items as zero when summing up, filter them out in the first place. As a bonus, this makes this case more parallel to the `isTopicNarrow` case just below.
This deduplicates a bit of code between these two very similar codepaths.
Use `caseNarrow` to let the specifics of narrow "operands", etc., stay an implementation detail of the `Narrow` type. This also gives us type-checking that we've thought of all the types of narrows here... and exposes that we had not in fact thought of the all-PMs narrow! That's purely a latent bug: despite its general name, this selector is used only to feed one part of the UI, namely the "N unreads" banner above a message list, and we don't expose the all-PMs narrow as a message list you can actually view. But it's good to make explicit so we aren't surprised in the future. The diff is best read with `git diff -b`, as it causes some re-indents.
Instead of using `narrow[0].operand` and hoping the code correctly lines up its meaning with the conditionals, use the `caseNarrow` family to unpack the components of the narrow in a structured way.
As the jsdoc added here says, all these call sites should probably be doing something else: where the code is really about a stream, or a stream conversation, it should be getting passed down a stream name, or stream name and topic (better yet, a stream *ID* and possibly a topic) in the first place, rather than a Narrow. Where on the other hand the code naturally takes a whole Narrow because it handles other kinds of narrows too, it should generally be using caseNarrow. Fixing that would be its own set of refactors in a mostly independent direction from this one, though, so for now we leave that for a future project (or really series of small projects.)
Instead, use the getters and predicates that make up the `narrow` module's actual interface.
This conversion is a no-op. But we'll shortly be converting `Narrow` into an opaque type -- which will make it a no-op that only the `narrow` module can perform. That means Flow will check for us that we've explicitly marked all the places where we intend this conversion to happen; these are those places.
In the preceding series of commits, we've taken all the places where our code used to depend on the details of our internal representation of narrows, and converted them so they use reasonable explicit interfaces provided by the `narrow` module. This commit recruits Flow to confirm for us that that job is complete. Because `Narrow` is now an opaque type, code outside this module now has to treat it as if it could mean anything: so it can't create a value of this type any more than it could of type `empty`, and can't consume one or break it down any more than it could a value of type `mixed`, the supertype of all types. Effectively this means that code outside `narrow` can only create a `Narrow` by invoking something inside the module; can then pass it around and store it however it pleases; but then can only inspect the information inside it by passing it back to something within the module, or to an operation like `===` or `JSON.stringify` that's completely generic so that it actually accepts type `mixed` as input. (The way this series was developed is that this commit came first, immediately after distinguishing the `ApiNarrow` type and making `Narrow` an alias of it in the first place. That produced a long list of Flow errors, which served as a to-do list to make the other changes in the series. Once complete, this commit was rebased to the end so that, as always, no commit in the series breaks our tests.)
chrisbobbe
force-pushed
the
pr-narrow-opaque
branch
from
194a0ae to
1ca184d
Compare
|
Great! Thanks again; merged. |
This completes a major objective of the long string of refactoring that appeared in the series of PRs zulip#4330, zulip#4332, zulip#4335, zulip#4339, zulip#4342, then zulip#4346, zulip#4356, zulip#4361, zulip#4364, and most recently zulip#4368. After this change, the portion of zulip#4333 that's about PMs, emails, and user IDs -- aka the portion of zulip#3764 that's about narrows -- is complete. Still open from zulip#4333 is to convert stream and topic narrows from stream names to stream IDs. I'm hopeful that that will be simpler: (a) unlike the situation for PMs, there's just one stream mentioned at a time, so there's no question of sorting, and (b) there isn't the "include self or not" complication that's bedeviled much of our code related to PMs. In other words, stream and topic narrows don't suffer from zulip#4035.
2 participants
In this series, we take all the places where our code used to depend on the details of our internal representation of narrows, and convert them so they use reasonable explicit interfaces provided by the
narrowmodule. Along the way we introduce several new such interfaces that we hadn't had before, and simplify some of the calling code.At the end of the series, we recruit Flow to confirm for us that the conversion is complete, by turning
Narrowinto an opaque type (aliasing its real internal representation) so that code outside the module cannot create narrows or see inside them.This follows on the recent previous PRs #4339, #4335, #4330, and #4329, and sets us up to be able to address #4333, changing the internal representation of narrows so that it identifies users and streams by their numeric IDs, rather than emails and stream names. I have a draft branch on top of this one which does the user part of that migration.