user: Convert to UserItemById from whole-User-taking UserItem #4404
Conversation
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! One comment below.
src/reactions/ReactionUserList.js
Outdated
| return <UserItem key={user.user_id} user={user} onPress={this.handlePress} />; | ||
| }} | ||
| renderItem={({ item }) => ( | ||
| <UserItemById key={item} user={item} onPress={this.handlePress} /> |
There was a problem hiding this comment.
17bf4a5 user: Use UserItemById in ReactionUserList; simplify whole call stack.
Hmm, I'm seeing that UserItemById's props aren't getting type-checked; do you see that too? I notice that the user prop given here is a number, and that the userId prop isn't given. I also notice that the callsite in GroupDetailsScreen does pass a number for userId (that sounds right to me), and the callsite in PmConversationList gives a number for user and doesn't give userId.
I believe the intended interface is that callers pass userId (a number) and don't pass user because that'll be supplied (as a UserOrBot) by connect.
I also find that I can give, e.g.,foo="nonsense" in one of UserItemById's callsites without getting Flow errors.
There was a problem hiding this comment.
Hmm! Thanks for catching this. Will investigate.
|
New revision pushed! |
chrisbobbe
force-pushed
the
pr-userids
branch
from
f66cfc4 to
93a8688
Compare
|
Great, thanks! I've just rebased across the changes in #4393, which was just merged, and I pushed the result to this PR branch. There were a few moderately sized conflicts, so I wonder if you'd like to take a quick look and spot any rebase errors I might have made? I don't expect any, but you might like to double-check. Otherwise, please merge at will! |
There seems to be a Flow bug where we weren't getting effective type-checking of calls to this component -- a callsite could pass arbitrary props, and fail to pass required ones, and Flow wouldn't complain. See discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20and.20.60connect.60/near/1098203 We can fix that by explicitly providing the props type, rather than using our `connect` with its subtle generic types that effectively ask Flow to infer it in a particular way. Moreover, while we're here, convert this `connect` call into a Hooks-based component, using `useSelector`. We still need the wrapper component, unlike the typical `useSelector` idiom, because we still have callsites invoking the inner component directly. But even so, given all the type annotation we had to provide to `connect` in this case, the `useSelector` version is a bit simpler. See also zulip#3768, where we ran into a very similar set of symptoms which might involve the same underlying Flow bug. Reported-by: Chris Bobbe <[email protected]>
This is one of the followup conversions I mentioned in zulip#4364, where I recently introduced this component and the first use of it.
This is a nice chain of simplifications we get from this conversion.
Continuing the conversion to UserItemById; and cutting a reference to email along the way. It'd be nice to go on to propagate this through the helper `groupUsersByStatus`, but that can't trivially be done: it uses the presence data, which is indexed by email. We'll naturally revisit this when we take care of migrating presence to user IDs.
Another step in the conversion to the UserItemById component. This one is particularly nice because it saves us from keeping around a list of `User` objects as local state. That means we automatically fix a whole class of glitch-level bugs that this code must have had, where when some details about a user changed after you'd already chosen to send your sharing message to them, the information we showed about them wouldn't update as normal.
The thing we're doing here in the autocomplete suggestions probably isn't *great*, or the ideal UI, and certainly makes a bit of a mess in the code. But doing better is a different kind of change from the refactoring we're engaged in in this series. For now, just give an appropriate name and comments, to keep the mess isolated to this one callsite. We'll next rename things so that the new UserItemById is simply UserItem, the default export; that'll be a separate commit.
This small migration is now complete, except for one recalcitrant callsite of the old API. (Which isn't causing us to hang onto any appreciable amount of code to support it, fortunately.) So, give the new preferred thing the nice simple name.
|
The rebase looks good, thanks! Merged. |
2 participants
This is the small series of followup conversions I mentioned at #4364. Not on a critical path to our major data-refactoring priorities, but a small nice cleanup.
The branch doesn't end up eliminating the old UserItem. (Or making private, really -- the code wasn't going to be deleted in any case, because it does most of the actual work of the new UserItem, which just wraps it with
connect.) That's because it turns out that in one place, the autocomplete suggestions, we actually make use of the ability to supply fake synthetic users that don't exist in the users data.That's likely not the best way that could work, at either a UI level or a code level. But doing anything about that would be a bigger project than this series otherwise is, and doesn't seem like it has a big payoff compared to the priorities we're working on, so we leave it be for now, and just rename and re-doc things to isolate it to that one spot.