Prep commits for new `avatar_url` handling (#4157) #4171

chrisbobbe · 2020-06-24T05:10:01Z

In a revision of my work for #4157, I noticed that there were some necessary changes to some test files, and some of these files haven't been type-checked or taken advantage of the work we've put into src/__tests__/lib/exampleData.js.

Rather than continue editing these files in this unstable state, pave the way for better edits by type-checking them and using that example data. 🙂 And fix a few bugs I've noticed.

The revision that informed these changes was one where the type for avatar_url on the Message type (not just that field on the User type) was changed. I think this might be a direction we'll go in (e.g., the type might be a union of all subclasses of AvatarURL except IdleAvatarURL); I'm thinking of the discussion around here. In any case, that explains why fetchActions-test.js was touched (fetchMessages is in there).

There will be some more of this; I know at least src/webview/__tests__/webViewHandleUpdates-test.js has some stuff that I'm still figuring out.

chrisbobbe · 2020-06-24T18:29:23Z

flow-typed/redux-mock-store_v1.2.x.js

@@ -5,12 +5,15 @@ declare module 'redux-mock-store' {
  */

  declare type mockStore = { <S, A>(state: S): mockStoreWithoutMiddleware<S, A>, ... };
-  declare type DispatchAPI<A> = (action: A) => A;
-  declare type Dispatch<A: $ReadOnly<{ type: string, ... }>> = DispatchAPI<A>;
+  declare interface Dispatch<S, A> {


Mm, the S type argument is unnecessary (making a comment so I remember to address this in my next revision, along with code-review feedback).

(and a code comment here saying that we've edited this bit would be good too)

chrisbobbe · 2020-07-08T23:53:32Z

Just resolved some conflicts; awaiting review. 🙂

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]

chrisbobbe · 2020-07-14T21:19:44Z

Just rebased again, following the RN v0.61 upgrade (no conflicts), awaiting review. 🙂

chrisbobbe · 2020-07-14T22:23:47Z

Ah, oops: Just found a few things to do better, so I pushed again; hopefully this didn't interrupt a review. I'll move on to something else now so you don't have to worry about possible interruptions again, and I'll post here if for some reason I need to make more changes.

chrisbobbe · 2020-07-20T21:57:11Z

Mm, looks like there are some conflicts—fixing those now. edit: Fixed.