per-account 6/n: Stop letting GlobalState cast to PerAccountState #5105
Conversation
| * | ||
| * TODO(#5006): We'll have to fix and eliminate each call to this. | ||
| */ | ||
| export function dubPerAccountState(state: GlobalState): PerAccountState { |
There was a problem hiding this comment.
I'm reading "dub" as a synonym of "assumeSecretly" in assumeSecretlyGlobalState. That's right, isn't it?
I'm not saying we need to replace "dub" with "assumeSecretly" in either of these. That'd make for some long names, and both will go away soon. But I wanted to check my understanding.
There was a problem hiding this comment.
It basically is. I had a distinction in my mind when I picked a different name here... but I think at the end of this PR, where GlobalState is no longer a subtype of PerAccountState any more than vice versa, the distinction no longer means anything, so probably it wasn't helpful in the first place.
(I think the idea was that because at the time GlobalState <: PerAccountState, it wasn't "secret" but openly there in the types that a state: GlobalState was in fact a PerAccountState, and the function was just a way of making that conversion explicitly.)
| @@ -434,6 +434,8 @@ export type PerAccountState = $ReadOnly<{ | |||
| ... | |||
| }>; | |||
|
|
|||
| export opaque type PerAccountState: PerAccountStateImpl = PerAccountStateImpl; | |||
There was a problem hiding this comment.
redux types: Stop letting GlobalState pose as PerAccountState
In particular (and I'm not sure if this needs a mention in the commit, I just want to check my understanding), this prevents GlobalState from posing as a PerAccountState in code outside this file. We totally can do that within the file, and in fact we do:
// For now, under our single-active-account model, we want a GlobalState
// to be seamlessly usable as a PerAccountState.
(s: GlobalState): PerAccountState => s; // eslint-disable-line no-unused-expressionsThere was a problem hiding this comment.
Yes, that's right. Mmm, and I should update that comment and/or cast, too, because they become misleading with this change.
gnprice
force-pushed
the
pr-peraccount-part6
branch
from
8581298 to
41f7413
Compare
This is a global thunk action -- it starts by explicitly looking at all the accounts -- but then invokes some per-account selectors, meaning to do so on the active account's per-account state. Make that transition explicit, so that we can stop letting a GlobalState masquerade as a PerAccountState. Marking that transition also brings up that we should explicitly deal with the case where there is no active account (because there is no account.)
These will let us explicitly mark places where for now we'll still take a GlobalState and use it as a PerAccountState, so that we can tighten down those types in general.
…unt state This is the spot where this assumption is fundamental to how our schema currently works, and is expected to be the one last remaining fixme of this kind that gets deleted at the same time as we actually change the schema so that the per-account and global states are different objects.
This global component is using per-account state, from the active account, to control some global UI -- the badge count on the app's icon in the launcher, for some Android varieties that have such a thing. The mismatch of types actually highlights how the feature is pretty broken if you use multiple accounts, in that the count reflects only the one last account you used. Perhaps we should just cut the feature. In any case, for now, mark the type mismatch so that we can tighten up these types in general.
This may be easiest to take care of after actually migrating session and settings to separate them per-account vs global.
Well, without fixmes, anyway. Most of which (just a handful) we've added in the preceding few commits. In this commit we add two fixmes as plain fixme comments, rather than calls to nice explicit functions. One is adjacent to an existing dubJointState call and will naturally go away when that one does; and another fixme in reducers.js, which will be easier to resolve when we do some re-organizing of that code to distinguish per-account vs. global reducers, coming soon.
gnprice
force-pushed
the
pr-peraccount-part6
branch
from
41f7413 to
85ecbb4
Compare
|
OK, merged. Thanks for the review! I fixed the second thing, but I let the confusing asymmetry of "dub" vs. "assume" stand for now. Once I finish applying the per-account vs. global distinction generally, with the remaining phase of that being actions, the next bundle of work for me in this direction will be cleaning up one by one the specific remaining places we break it, most of which are calls to those functions. So as part of that cleanup I might rename for more symmetry. |
This is the next PR in the series after #5083, #5066, #5030, #5016, #5017, and #5023, produced from the branch described at #5006 (comment).
From the foreshadowing last time:
That's this PR.
Then, continuing as described last time: