types: Use a distinct UserId type for user IDs.#4421
Conversation
That is, construct it more systematically, and get data on test users from the exampleData code.
Many of these can't possibly work in the first place -- how can we edit a user group, for example, without providing any data on what we want to change about it? In any case, we don't use them, and have no plans to in the short or medium term. When we do, it's not hard to add bindings, and really that will only be made easier by not having broken ones lying around.
The imports belong to the whole file, and this had the import placed inside the first section of it.
We'll use this in waves over the coming series of commits. Because it's public that `UserId` is a subtype of `number`, other code can freely take a `UserId` and use it as if it were simply a `number`. That means we can do the conversion gradually so long as we start with places where a user ID is made, and follow the data flow to places where it's passed to and ultimately consumed: while we're in the middle of the conversion, the boundary will always look like some code providing a `UserId` and some other code accepting it as a `number`.
In places where these test helpers accept a user ID, we'll keep accepting plain numbers, to just reduce the number of places where test code has to say `makeUserId`. But where they provide a user ID, start specifying that it's not just any number but a genuine `UserId`.
Where we make up a user ID in test code, mark it in the type system as being a user ID and not just any old number. We'll soon adjust the types of our many functions that expect data containing a user ID, so that they require this marking. This change prepares for that.
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Great, thanks!
See also a small comment and a query, below; otherwise, this LGTM.
| WebViewInboundEventReady, | ||
| WebViewInboundEventMessagesRead, | ||
| } from '../generateInboundEvents'; | ||
| import { makeUserId } from '../../api/idTypes'; |
There was a problem hiding this comment.
We should run tools/generate-webview-js at this commit.
There was a problem hiding this comment.
Indeed, thanks for the catch!
| */ | ||
| export const makeUserId = (id: number): UserId => id; | ||
|
|
||
| /* Possible future work: |
There was a problem hiding this comment.
Possible future work:
Makes sense. Do you think it'll always be pretty clear whether a given ID type belongs here in src/api/idTypes.js, vs. somewhere else?
I'm thinking of the return value of keyFromNarrow, which we've discussed making an opaque type for. I think that pretty clearly belongs right alongside keyFromNarrow, in narrows.js. I think I'd say it doesn't belong in src/api because we never pass those keys to the API, and we never receive them from the API. We do pass the result of apiNarrowOfNarrow to the API.
I think this is a sensible criterion for putting ID types in src/api/idTypes.js: if we ever talk to the API using that type of ID, its opaque type should live here. Perhaps that goes without saying, but we do a lot of passing-around of IDs within the app (including of user IDs, stream IDs, and our own narrow keys), so I wonder if a small comment might be helpful. 🙂
There was a problem hiding this comment.
The basic principle is that src/api/ is self-contained, so that code there shouldn't import code elsewhere -- it's describing the server API, and behaves like an independent library for doing that. We've made a few exceptions to that principle for convenience, but are generally pretty consistent about it.
So that means that any type that's going to be used in the API should be defined in the API part of the code. If we introduce a StreamId or MessageId, we'll want to use it in the API, so it'll be defined there.
The narrow-keys returned by keyFromNarrow don't appear in the API; they're our own invention for within the app. So they naturally wouldn't be defined in src/api/, and instead would appear in narrows.js as you say.
Because this is a more general principle I don't think it calls for a comment on this specific instance. But it'd be good to write down that general principle, which I'm not sure we have. Any suggestions on where would be most helpful?
There was a problem hiding this comment.
Makes sense, thanks! I've just merged ba7108f, wiki-style.
This NULL_USER object should really go away, and perhaps it will soon. Until then, mark this fake user-ID value of -1 as indeed being meant as a (fake) user ID.
Specifically, use it in all the places that are purely an "output position" (or nearly so), i.e. where we consume a value from this type but don't try to put a value back into it. This leaves out a few parts of the API we'll get to in subsequent commits: * a few places which are "input positions", meaning the API code accepts values of this type instead of providing them -- in particular when we're providing user IDs as parameters; * a few places where we reuse the type for new values we construct for our own internal data structures, so that the type is in an input position there as well as an output position. Those will require updating more of the app too, so that we can supply UserId values.
This PmRecipientUser type is one we reuse in some data structures we construct ourselves, in particular outbox messages. So to tighten its type, we need to propagate the fact that these values are user IDs through the various code that handles them on their way there, in particular the Narrow type and its constructors.
This propagates the UserId type through where we store in Redux the self user's user ID, to the `getOwnUserId` selector that gets that information, and on to all the values fed by that selector. We already have a pretty consistent convention of calling those values `ownUserId`, so this is an easy change to make.
This change in the API types is tied together with a small change in our code that consumes it, for a bit of a silly reason: we want to sort this array, and we'd rather do so in place rather than making an unnecessary copy, and that means it can't be `$ReadOnlyArray`. In principle this could be avoided by Flow having something intermediate between plain `Array` and `$ReadOnlyArray`, with a name like `$CovariantArray`. Like `$ReadOnlyArray`, this would exclude methods like `push` that accept a new item, and that therefore aren't safe to use on an array that some other code might expect to have a more specific type of element. But unlike `$ReadOnlyArray`, it would include `sort`, and `pop`, and other methods that mutate the array but only provide elements as output, never taking them as input, and therefore are just fine for that situation. But Flow doesn't currently have that feature, so just treat this type covariantly and update both the provider and consumers in lockstep.
We do this only partially, due to a Flow bug in the interaction of indexer types (i.e. the types of objects-as-maps) and opaque types.
We reuse this PmsUnreadItem type for the data structures we maintain over time for this data, so tightening its type requires us to adjust a type in the code that maintains it.
This makes the meaning of the `unsubscribedMentions` state-property rather clearer! The elements are user IDs.
All these changes are pretty boring. Many of them are independent of each other, but others aren't because one component passes a user ID to another, either as a child or by navigation. This change is limited to the places where a user ID, or list of them, appears in order to specify particular users that this component, this time, should do its work with respect to. Upcoming commits will take care of places where we pass general information that happens to contain user IDs.
The data we're passing to this code comes from PM recipients, where we've already marked the user IDs as type `UserId`. So when we tighten the type here, Flow already accepts that. This in turn causes Flow to see that this code's local helper `getRecipients` operates on `UserId`s. It passes the user IDs to one of our Redux selectors; so in an upcoming commit, when we tighten those selectors to expect `UserId`, Flow will see that this change is in fact required.
This comes after converting React props and route props, which provide the arguments to these selectors at most call sites, and before converting the `get*UsersById` selectors which provide the data structures these look up data in.
In this commit, we update the selectors that return maps of users by ID to mark the keys as being specifically user IDs, not just any numbers; and we propagate that change through all the values that just pass on these selectors' return values.
…ion. This takes us full circle from the early part of this series, where we started using UserId where the API provides a user ID. After this change, a search like `rg -i user.*number`, or similar searches with "recipient" or "sender" instead of "user", turns up only a handful of places where we have something intended to be a user ID that's still of type `number`; and each of them has a comment explaining why.
gnprice
force-pushed
the
pr-userid-type
branch
from
855cf8a to
1680f7b
Compare
As Greg mentioned at #4421 (comment). Possibly, there's a better place for this -- but I'd like to put it somewhere before I move on to other things. So, wiki-style, I've taken the liberty of merging this small change, which purely fills a gap in our comments/documentation.
As Greg mentioned at zulip#4421 (comment). Possibly, there's a better place for this -- but I'd like to put it somewhere before I move on to other things. So, wiki-style, I've taken the liberty of merging this small change, which purely fills a gap in our comments/documentation.
2 participants
This makes a new
UserIdtype as a Flow opaque type alias, like so:The effect of this is that:
UserIdis simply anumber, with no added overhead;UserIdcan be freely converted to a plainnumber(because of the: numberbound);UserIdfrom a plainnumbercan only be done by code in the same module as the type definition. The effect of this part is that it requires invoking an explicit functionmakeUserIdwhich we export from that module.This requires touching a lot of code and, frankly, I'm not sure it was worth the effort required to prepare the changes. But with that work done, I think it definitely makes the code nicer than it was without it. It moves one more set of facts about our code out of the realm of unchecked commenting and naming, and into the realm of machine-checked types.