per-account 4/n: Sort out thunk actions #5066

This function has a bit of logic that makes sense only in a global context (asking whether there even is an account to have state for), but then the bulk of it is per-account. One group of its callers is firmly per-account, in the initial fetch in fetchActions.js. So those would like this function to be per-account. But then its other caller is withHaveServerDataGate. It's possible that should also be per-account, but I'm not sure of that -- in particular I'm not sure we only use it in contexts where there is an account at all. Or possibly that should be the case but it would take some refactoring to make it so. For the present, then, I'd rather defer that question by not changing its behavior; I want to just keep it making all the same checks it currently does. To accommodate both sides of that, we split this into a global and a per-account version, and have each caller use the appropriate one. This means those per-account actions in fetchActions.js are now calling the per-account version of this selector.

This is really a per-account thunk action, which we'll soon be giving only a per-account state. So this bit of logic where it's assuming that that's also secretly a global state will need to be marked like this.

These helpers' callers are per-account thunk actions. We'll need this when we make plain ThunkAction stop getting a GlobalState.

In the old version with `$Diff`, Flow wouldn't complain if a component expected a `dispatch` prop with too specific a type for what it was actually going to get: that is, if the type it expected was a subtype of the type actually provided by `connect`, encoding more-specific expectations about the value than `connect` was actually promising to meet. It actually *would* complain if the component didn't have specific *enough* expectations of the type. That is, it'd accept calling `connect` on a component expecting a `dispatch: empty` -- something `connect` couldn't possibly give it -- but reject doing so with `dispatch: mixed`, which should be perfectly fine. Effectively, by using `$Diff` here, we seem to have gotten the comparison backward. (And Flow is unable to check our work in doing so, because the whole reason we have this file is that the react-redux libdef is too lax.) That hasn't really been a practical issue to date, because the only reasonable type for something called `dispatch` in our codebase has been `Dispatch`. But we're soon going to have two flavors of that type, one for acting on a particular account's data and one for acting on all accounts' or purely account-independent data, so the distinction will become an important one. Happily, our `BoundedDiff` solves this problem.

… type This component discards the `dispatch` prop it gets, so it doesn't really care about its type and can say it merely expects a `mixed`. This will let us change the `dispatch` type that gets passed here without having to make a meaningless edit at this line. Previously we couldn't write `mixed` here because of a bug in our `OwnProps` type, which is used in our `connect`. We've just fixed that by using our `BoundedDiff` in place of `$Diff`.

We're about to have two different "dispatch" types. This will let us use the same `OwnProps` with reference to either of them.

For now the plain Dispatch still provides a GlobalState (which in turn can still for now be used as a PerAccountState). With these separate names we'll be able to start distinguishing thunk actions that really are meant to be global from those that are per-account. Then we'll be able to tighten the plain ThunkAction type (and Dispatch, which is tied to it) to provide only a PerAccountState.

This handy feature of redux-thunk lets us provide a way for per-account thunk actions to get access to data that's legitimate for them to have but lives outside of a PerAccountState. We leave a fixme in the redux-mock-store libdef, with a TODO comment so we'll know to come back to it later in this series of PRs. The libdef already had an imprecise type here, and it's for a module only used in tests so it's not too critical if there's a type error somewhere that it ends up concealing.

This marks as global all our global thunk actions and some of their callers. The remaining callers are a couple of Redux components. For now, they still invoke these thunk actions with a plain `Dispatch`, and so we rely for now on the fact that we haven't yet made that distinct from `GlobalDispatch` -- the former still provides a `GlobalState` to its actions. In the next commit, we'll make it possible to mark those components as global so that they get a `GlobalDispatch`, and then we can tighten `Dispatch`.

This marks as global all the places we get a `dispatch` which is meant to be global. For now the distinction between `connectGlobal` and plain `connect` is only about the `dispatch` prop, not the state provided to the `mapStateToProps` callback; so plain `connect` still provides a GlobalState. We'll tighten that part separately later.

This confirms that we've now explicitly marked global all the places that we need a GlobalDispatch. Anywhere that's still getting a plain Dispatch -- which means the bulk of our thunk actions and Redux components -- has to actually behave as per-account, modulo any explicit fixmes. (And we've only added one of those, a call to assumeSecretlyGlobalState in eventActions.js.)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

per-account 4/n: Sort out thunk actions #5066

per-account 4/n: Sort out thunk actions #5066

Commits on Oct 26, 2021