redux: Distinguish per-account vs global state (1/n) #5017
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also add a small function assumeSecretlyGlobalState, to explicitly coerce a PerAccountState to GlobalState; we'll do that in a few places as scaffolding while we're converting things over. And add a tiny type-test that converting in the other direction can be done implicitly. In the middle of the transition, we'll be doing that a *lot* -- for example at most places we call a selector from a thunk action or a React component, when we've converted selectors but not yet thunk actions or React components -- so it'll be quite handy that it can be one-way implicit like that.
This doesn't yet have any real effect, because for the moment PerAccountState and GlobalState are aliases.
chrisbobbe
reviewed
Sep 22, 2021
| import { getRawRealmEmoji } from '../directSelectors'; | ||
| import { getIdentity } from '../account/accountsSelectors'; | ||
| import zulipExtraEmojiMap from './zulipExtraEmojiMap'; | ||
| import { objectFromEntries } from '../jsBackport'; | ||
|
|
||
| export const getAllImageEmojiById: Selector<RealmEmojiById> = createSelector( | ||
| getIdentity, | ||
| state => getIdentity(assumeSecretlyGlobalState(state)), |
There was a problem hiding this comment.
redux types: Add two assumeSecretlyGlobalState markings
Just to check my understanding: Is it intentional that getIdentity is still a Selector at the time this change is made, and the switch to GlobalSelector comes after?
In the commit message, I see "[…] invokes one of those selectors (which demands a GlobalState)", and I think I'm a little confused because at this commit getIdentity doesn't yet demand a GlobalState, by that name, anyway.
There was a problem hiding this comment.
Yeah, I guess that is kind of talking about the future. Probably it'd be clearer to switch things to GlobalSelector where appropriate, then add these two markings, then switch other selectors to PerAccountState where appropriate (and then make the change that distinguishes PerAccountState from GlobalState.) I'll quick make that change.
| * on mobile at all, which is what will happen soon if the user | ||
| * doesn't act on the notice. | ||
| */ | ||
| hasDismissedServerCompatNotice: boolean, |
There was a problem hiding this comment.
Cool! This looks like progress toward solving a pretty small bug, but a bug nonetheless.
From #4750 (comment):
NOTE: The has-dismissed-the-banner state isn't stored per-realm, so there's currently a bug where dismissing the banner makes it go away for all <2.0.0 realms the device knows about (until app restart, anyway). I figured this was minor enough to be acceptable for the time being, unless we can think of a nice and easy way to store the state both per-realm and per-session, while being sure to clean up after ourselves when the user wants to forget the realm.
There was a problem hiding this comment.
(I think per-account is a totally fine proxy for per-realm.)
We're in the process of marking selectors as either per-account, taking a PerAccountState, or global, taking a GlobalState. In this commit, we apply the GlobalSelector alias where it's needed. In a couple of commits, we'll address selectors that don't use that alias; they currently all say GlobalState, and we'll switch to PerAccountState where appropriate. We actually go farther here than we ultimately want to: we mark all the `state.accounts` selectors found in `accountsSelectors` as global. In fact these are a mix of global and per-account, mostly per-account; but teasing those apart will take some careful sorting-out of their callers' expectations, so we'll leave that to a bit later in this series of PRs. For now we leave them all marked as global, which is the conservative option in that it means Flow notices anytime we call one of them from a per-account context. This doesn't yet have any real effect, because PerAccountState is for the moment just an alias of GlobalState.
In a bit, we'll make PerAccountState and GlobalState actually be distinct, and we'll mark most of our selectors as per-account or global accordingly. The major area of selectors we'll skip at first, because it requires some careful sorting-out of what its callers expect of it, is the selectors on `state.accounts` found in `accountsSelectors`. We'll initially say those are global selectors, reflecting the data that (like all our selectors) they have access to now in the status quo. Because we'll have GlobalState freely implicitly convert to PerAccountState (for a while) but not vice versa, that means that Flow will notice any place that a per-account selector (which has only a PerAccountState) invokes one of those selectors (which demands a GlobalState). We don't have many of those, but we have these two. In each of these cases, the calling selector really is working on a particular account's data, and although the data it's getting from the callee selector is stored in `state.accounts`, it really is specific to the active account; and in a multi-account future, the data we'll want here is really from whatever account the caller is working on, regardless of whether it's the active account in the UI. So these selectors are perfectly well doing the right thing; we just need to add these assumeSecretlyGlobalState markings, to tell Flow that we know about the type mismatch here and we're on it.
After this change, the bulk of our selectors are correctly marked as per-account, taking a PerAccountState -- which is most of them -- or global, taking a GlobalState. The major class of exceptions is the `state.accounts` selectors found in `accountsSelectors`, as discussed in a previous commit. A more boring class of exceptions is the many small anonymous selectors (mostly as callbacks to `useSelector`) that get bits of the settings and session data. Many of those are really per-account, but for now we leave them all marked as global. At this stage none of our thunk actions or React components get converted. That means they're all still acting on GlobalState; which is why it's very convenient that (for the next while) we've set up PerAccountState so that it's a supertype of GlobalState, and a value of the latter can be freely implicitly used as the former. (Of all the Redux-related names we'll be splitting, this one is unusual in requiring renames through the per-account majority of the app code. Most will be like Selector in that the existing name is short and generic and we'll keep that for the per-account majority to use. GlobalState is the main exception, because the existing name is explicitly "global".) This doesn't yet have any real effect, because PerAccountState is for the moment just an alias of GlobalState. The real test that we've assigned each of these to the proper column will be that Flow passes when we make those two types distinct, coming soon.
Now the fact that Flow still passes means that we don't have any place where we have a PerAccountState, and try to pass it off (without an explicit assumeSecretlyGlobalState, that is) as a GlobalState.
gnprice
force-pushed
the
pr-peraccount-part1
branch
from
b9f1900
to
ab805f2
Compare
|
Thanks for the review! Merged. |
This was referenced Oct 4, 2021
This was referenced Oct 21, 2021
This was referenced Nov 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the second PR in the series that began with #5016, produced from the branch described at #5006 (comment) .
At the end of this PR, we have a type PerAccountState which is distinct from GlobalState, exposing only the per-account parts of the data. A GlobalState can be freely used implicitly as a PerAccountState, but not vice versa. And to start exercising the distinction, most selectors have been marked as per-account, accepting a mere PerAccountState -- which also means that any selectors they go on to invoke must also be per-account.
The next steps in my draft branch after this PR are to convert the remaining selectors to be per-account where appropriate. Mainly this means those consuming
state.accountsinaccountsSelectors, which involves teasing apart just what's expected at their various callers -- whether they truly want "the active account", or (like the bulk of our other selector callsites) want "this account", i.e. the particular account the caller is already acting on, which is just assumed to be the active account because that assumption typically holds across our codebase.After that, we'll convert thunk actions and React components so that they get a PerAccountState rather than a GlobalState.