Make state.narrows an Immutable.Map.
#4187
Conversation
chrisbobbe
force-pushed
the
pr-narrows-immutable-map
branch
from
8e66538
to
deba044
Compare
chrisbobbe
changed the title
state.narrows an Immutable.Map.state.narrows an Immutable.Map. 🎉
chrisbobbe
changed the title
state.narrows an Immutable.Map. 🎉state.narrows an Immutable.Map.
chrisbobbe
force-pushed
the
pr-narrows-immutable-map
branch
from
deba044
to
1a80243
Compare
|
Just resolved some conflicts; this is ready for review again. |
There was a problem hiding this comment.
Neat!
Before actually merging the headline change, I want to spend a bit more time looking at it, and playing around with it and thinking about ways to test.
But clearly quite a lot of the work of this was in the commits to get all those tests well-typed. I've read through all those now, and I'll go ahead and merge them!
| const anchor = getMessagesForNarrow(state, privateNarrow('john@example.com')); | ||
|
|
||
| const expectedState = deepFreeze([{ id: 123 }]); |
There was a problem hiding this comment.
Much better -- thanks for cleaning these up 🙂
I think with these names you're looking at the results of two different cases of someone copy-pasting without thinking it through (or perhaps not understanding.) The expectedState is, as you say, a reasonable name in many reducer tests.
The anchor... I suspect comes from the last few test cases below, testing things like getFirstMessageId. There, the result is at least plausibly an "anchor" -- it's a message ID that we're going to use to identify a spot in the message stream. And then those got copied elsewhere, by a contributor who didn't understand what it meant and in a period where we weren't doing meaningful code review.
OK, and having said that I was curious to find out. 🙂 Indeed -- and another layer is that those tests for getFirstMessageId etc. were originally for a function called... getAnchor. a89d897 changed that. And before then, 8dd2081 propagated the anchor name to these other test cases.
| display_recipient: [ | ||
| eg.displayRecipientFromUser(eg.selfUser), | ||
| eg.displayRecipientFromUser(userMark), | ||
| eg.displayRecipientFromUser(userJohn), | ||
| ], |
There was a problem hiding this comment.
These are probably a sign that it'd be helpful to refactor eg.displayRecipientFromUser to be called, say, eg.displayRecipientFromUsers -- and to take User[] and return PmRecipientUser[], and be just a map around what it currently is.
(Maybe even both versions should exist.)
gnprice
force-pushed
the
pr-narrows-immutable-map
branch
from
1a80243
to
9156a54
Compare
In an upcoming commit, we'll change the Dispatch type in this libdef to play nicely with our async redux-thunk actions. I don't see an easy way to give the libdef the flexibility to give different types depending on what middleware is used, so, we hard-code it for ourselves, since we know we'll be using redux-thunk for the foreseeable future. This means we probably can't reasonably make a PR to FlowTyped, even though this is (surprisingly) one of the few libraries we use that does have a libdef hosted by FlowTyped. Ah, well. Here, we just move the file, delete the metadata at the top, and run our auto-formatting. [cherry-picked from zulip#4171]
This version [1] is marked for a higher version of Flow than we're using now (v0.104 and above), but there aren't any errors on our Flow version (v0.98). So, take it. This gets us a particular change to the Dispatch type [2] that goes in the right direction...until we clobber it with our own Dispatch that accounts for async redux-thunk actions, I suppose. Ah well, there are a few minor changes that we might like to have, and this is a later version, so, might as well. [1]: https://github.com/flow-typed/flow-typed/blob/c4f47bdda/definitions/npm/redux-mock-store_v1.2.x/flow_v0.104.x-/redux-mock-store_v1.2.x.js [2]: flow-typed/flow-typed#3803 [cherry-picked from zulip#4171]
As foreshadowed in the preceding commits, we can't really expect a FlowTyped-hosted libdef to have the flexibility to give us the right types for Dispatch on the condition that we pass `thunk` as a piece of middleware. We're decidedly using `thunk`, so, hard-code support for it here by allowing dispatch to be called with a redux-thunk async action. We're pretty consistent with having correct types for such an action, so not much is lost by typing its arguments loosely as `Function, Function`, and from some attempts, it seems much easier than nailing down something exactly right. [cherry-picked from zulip#4171]
It looks like this was added with the idea that we'd use it in more places than we actually have. It adds a slight bit of convenience; with this mock, we can avoid having to write `configureStore([thunk])` a bunch of times. But that's a very small amount of boilerplate. The mock doesn't seem to have any other purpose that might be part of a unit-testing strategy. Removing it makes it possible to use the libdef without contorting it more drastically than we already have. [cherry-picked from zulip#4171]
The added fields were gleaned from a real-life example observed with `redux-logger`. [cherry-picked from zulip#4171]
Flow won't allow these inputs (a GlobalState of `undefined`, or an
action of `{}`).
…les. `expectedState` makes sense in tests for reducers, but not really for selectors. The *input* of a selector is the GlobalState, but the output is...whatever we want to do with that input. Sometimes we just pick things directly from the state; sometimes we do that and then nudge it around until it's in a shape we want. Also rename `anchor` to the more general `result`; I'm not really sure what "anchor" is meant to refer to in each of these instances.
We'll use this in pmConversationsSelectors-test in an upcoming commit.
gnprice
force-pushed
the
pr-narrows-immutable-map
branch
from
9156a54
to
afd013b
Compare
|
... Err, I guess that came out slightly wrong, in that this PR is marked as merged but with that headline final commit dropped, oops. Please go ahead and open a new PR with (a rebased version of) 1a80243. The set of commits merged to master were exactly what I intended, though 🙂 |
And show how to use it. [1] [1] zulip#4187 (comment)
|
Thanks for the review and merge! OK, opened #4201. |
And show how to use it. [1] [1] zulip#4187 (comment)
And show how to use it. [1] [1] zulip#4187 (comment)
An instance of #3949 and #3950, this is planned followup to #4047. See #4047 (comment).
Still getting all these tests type-checked, but thought I'd push my work today.🙂Done! Well, with a few things to note:
src/message/__tests__/fetchActions-test.jsisn't type-checked and made to use example data in this PR. Those are done in my current branch for Prep commits for newavatar_urlhandling (#4157) #4171, which is open. No need to cherry-pick that work, I think; that'll just make things more confusing. I don't think this has a strong influence on which PR we should aim to merge first: I just want to generally be sure that the test files I touch will be type-checked at some point.I did cherry-pick a few commits from my branch for Prep commits for new
avatar_urlhandling (#4157) #4171, only in support of type-checking other test files for the first time.I also cherry-picked one commit from Extend exampleData to further clean up messageActionSheet tests #3946, which is also open.Extend exampleData to further clean up messageActionSheet tests #3946 was recently merged, so no need—just rebased across this. 🙂I've called out all of these cherry-picks with a note at the bottom of the commit message.