Fix a recipients bug, preparing to use recent-PMs data
#4385
Conversation
There was a problem hiding this comment.
Great, glad to be getting closer to #3133! Small comments and a question below; otherwise, this looks good.
src/collections.js
Outdated
| * through the list again to look for nullish values. | ||
| */ | ||
| export function mapOrNull<T, U>( | ||
| ths: $ReadOnlyArray<T>, |
There was a problem hiding this comment.
Hmm, what does ths stand for? After reading the comment about thisArg, it makes me think it might have to do with that, but I think it doesn't.
There was a problem hiding this comment.
Ah, I should make it clearer, then. 🙂
It's meant to stand for "this" -- as in, if you imagine this were in the stdlib it'd be a method on Array, so the array it's mapping over would be this. But of course this is a keyword and I can't just call the parameter that, so I have to munge the name.
In Python there's a fairly common pattern that calls for a function to take an argument that is a class and you'd like to call it "class", and similarly class is a keyword and you can't. The standard solution there -- here it is right in the language reference docs -- is to call it cls. So I think in my head I was echoing that usage. But that's not so helpful for a reader who hasn't spent time writing Python 😉
Given the "thisArg" thing, probably anything that tries to say "this" will be confusing unless separately explained. Maybe I'll call it array.
| * called on any later elements. | ||
| * | ||
| * This replaces either (a) writing an explicit loop, in order to be able to | ||
| * abort on a nullish value, or (b) using `Array#map` and then running |
There was a problem hiding this comment.
abort on a nullish value
nit: abort on a null value
Hmm! Right, it does abort on a nullish value, not just null. The == null (==, not ===) makes it abort on a nullish value. I wasn't expecting the double-equals, but I suppose it makes sense to do one operation instead of two (i.e., r === null || r === undefined).
There was a problem hiding this comment.
Yep!
For the API choice, I think "nullish" is most helpful because it means it can work with things like Map#get that return undefined as well as with things that, like this function itself, return null.
For the code-style choice: I've gone back and forth, but my current leaning is that it makes sense to use == for basically this one pattern, of == null.
- It does exactly mean
r === null || r === undefined-- which makes it just about the only usage of==that can be described as cleanly as that. - It's rather shorter; and it avoids duplicating the expression, which doesn't matter here but can meaningfully reduce mess when it's more complex.
- It's a pretty tightly constrained and bright-line pattern -- so I think it's still quite doable to see
==in any context other than== nulland have that stand out as a code smell.
There was a problem hiding this comment.
(Similarly != null, naturally.)
|
|
||
| describe('mapOrNull', () => { | ||
| test('behaves right in happy case', () => { | ||
| expect(mapOrNull([10, 20], (x, i, a) => x + i + a.length)).toEqual([12, 23]); |
There was a problem hiding this comment.
This would be an odd way to use mapOrNull in the wild, right, but it catches all the potential pitfalls, and quite succinctly! Staring at it, I suppose its oddness is good because it makes me go and make sure I understand mapOrNull well. 🙂
| // prettier-ignore | ||
| /* eslint-disable no-multi-spaces */ | ||
| /* eslint-disable array-bracket-spacing */ | ||
| for (const [desc, users, expectedSet] of [ |
There was a problem hiding this comment.
nit: Maybe description instead of desc?; For a split-second I thought of descending because that's another thing that comes to mind when I look at arrays (though I may be biased from all the recent stuff about sorting).
Kind of puzzling that Flow doesn't complain about this one on its own! As ESLint points out in the rule that we have to disable here, the `= undefined` has no effect on runtime behavior -- it just states explicitly what the code was already doing anyway. But once we do make it explicit, Flow starts noticing that the `undefined` was propagating into boolean expressions and to an argument passed to `isSameRecipient` -- which also justifies a check at the top of that function which by its types had looked dead. Fix that function's signature, and booleanize the value when using as a boolean. This Flow behavior is concerning enough that we should probably just stop saying `let foo;`, and always explicitly initialize to `undefined` when that's what we mean. (The code is potentially a bit clearer that way anyway.) We'll do that next. I've also reported the Flow issue upstream, as: facebook/flow#8526
The remaining cases of plain `let foo;` are a few in src/third/, where we generally haven't been assimilating the style to our own.
If we'd had type-checking here in the first place, this test would have had user IDs in it, and wouldn't have been defeated when we stopped paying attention to the emails. Then when we stopped sorting here, we'd have noticed we were doing so and that we were introducing this bug. For the reason explained in the comment, I'd rather not just reintroduce sorting to fix this. The bug is low-severity and appears only in uncommon usage, so for now we just tolerate it.
This replaces a pattern I think we've written out explicitly a fair number of times, and which can be cleanly encapsulated instead. For now this is the only function in this new file, but I think we have a few other patterns with this flavor, which might be good to add functions for here too.
This has been a latent bug: the function wasn't living up to its documented spec. I believe it isn't a live one, in that we never actually pass it an input that triggers this -- all the representations for PM conversations that we deal with so far represent the self-1:1 thread with a singleton list, not empty. We're about to start handling the `recent_private_conversations` data structure, though, which does use an empty list of users for the self-1:1 thread. That turned up this bug; fix it. Also add some tests for this function, since it evidently needed them.
gnprice
force-pushed
the
pr-recents-prep
branch
from
29710da to
9d08464
Compare
|
Thanks for the review! Merged with those tweaks. |
I wrote up today a draft branch that will take care of #3133, replacing the previous #3535 and #4104, to use the server's handy
recent_private_conversationsfeature in order to populate the "PM conversations" screen both more completely and more efficiently.Along the way -- actually while rebasing and studying #4104, before I decided it'd be better to take a different approach than it or #3535 did -- I discovered a bug in
pmKeyRecipientsFromIds: it's supposed to work whether the input contains or excludes self, but it doesn't live up to that spec in the case of a self-1:1. This has been a latent bug because all of the representations for PM conversations that we deal with so far represent the self-1:1 thread with a singleton list, not empty. But therecent_private_conversationsdata structure does use an empty list for the self-1:1 thread, so it would trip on this bug and make it a live one.This PR fixes that bug, and also takes care of several other issues that that change turns up. (Including discovering a Flow bug which I then reported. Though that part was actually a couple of months ago -- I was looking at some of this code as part of #4299 and #4304, and that work led to the first two commits here and that Flow bug report.)