per-account 5/n: Sort out React components #5083
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
chrisbobbe
reviewed
Oct 27, 2021
| return useSelectorInner<PerAccountState, SS>(selector, equalityFn); | ||
| } | ||
|
|
||
| export function useGlobalSelector<SS>( |
There was a problem hiding this comment.
redux types: Split useSelector vs. new useGlobalSelector
A search for from 'react-redux' helps find two cases where the useSelector we're using is the one straight from react-redux, not our wrapper here that now uses PerAccountState.
After changing those two import { useSelector } from 'react-redux';s to instead say from '../react-redux', it looks like the useSelector in HideIfNotHydrated wants to be a useGlobalSelector.
There was a problem hiding this comment.
Hmm, good catch, thanks!
It looks like what happened in those cases is:
- One was a change written before we had the type-wrapper, and we didn't notice that point when rebasing and merging.
- The other was originally an equally mistaken direct import of
connect. We didn't notice in code review when merging, and didn't happen to notice later when converting to Hooks.
It'd be nice to have a lint rule against these imports, because it's an easy thing to do by accident (especially for contributors new to our codebase). I'll take a quick look to see if there's a real easy way to do that.
gnprice
force-pushed
the
pr-peraccount-part5
branch
from
afb459c
to
f1e89ab
Compare
|
Ack, mispush -- I "merged" this by pushing Will fix. |
These weren't using our type-wrapper. Noticed by Chris here: zulip#5083 (comment) It'd be nice to have a lint rule against these imports, because it's an easy thing to do by accident. First, fix these existing cases.
We'll need this fixme in order to make GlobalDispatch no longer a subtype of Dispatch.
This will allow this action to continue being dispatched from both global and per-account thunk actions, after we make GlobalDispatch no longer a subtype of Dispatch.
These didn't make much sense for GlobalThunkAction in itself, because they're both redundant when you have a getState that returns a global state. We've had them there in order to let GlobalDispatch be a subtype of Dispatch (and ThunkAction a subtype of GlobalThunkAction), in order to smooth the process of sorting out which code should get which of those. But that subtype relationship won't ultimately make sense either. It can only work as long as the getState that gets passed to a GlobalThunkAction is one that a ThunkAction can accept as its own getState parameter... i.e. only while `() => GlobalState` is a subtype of the `() => PerAccountState` that a ThunkAction expects... i.e. only while GlobalState is a subtype of PerAccountState. Which is true for now, where there's really just one per-account state at a time (for the active account), but can't work in a world with a real multi-account schema with per-account states for several accounts. So, we may as well drop these extraneous extras now. That completes the two-way distinction between Dispatch and GlobalDispatch, by making GlobalDispatch no longer a subtype of Dispatch. (Later in this series of PRs, we'll also make GlobalState no longer a subtype of PerAccountState, as part of our preparation for actually changing the schema.)
This makes useSelector return only a per-account state. In the relatively small number of places we need a global state from a selector, we mark that explicitly with a new hook useGlobalSelector.
This really is a per-account component. In particular it really wants a per-account `dispatch`, for the part where it actually reports presence to the server. That means it's correctly using the per-account form of `connect`, which will soon start giving it only a PerAccountState. There are several possible ways we could reconcile that with this line where it's using the global selector `getHasAuth`; one is in the comment added below. But for now, just mark this so we know we'll need to resolve it before we actually physically distinguish the global state from a per-account state.
…countState This is a per-account component, and this `connect` call will soon start providing only a per-account state. We'll need to fix one way or another the way it gets these bits of global state (all of which are perfectly legitimate for per-account code to be using), but for now just mark that.
With this, modulo fixmes, our React components are now sorted out into per-account and global.
gnprice
force-pushed
the
pr-peraccount-part5
branch
from
eeb9648
to
f85aa3c
Compare
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 next PR in the series after #5066, #5030, #5016, #5017, and #5023, produced from the branch described at #5006 (comment).
From the foreshadowing last time:
That's the second half of this PR.
The first half tightens up the typing on thunk actions, which were the subject of part 4 (#5066): we had things so that a
GlobalDispatchcould be silently used as aDispatch, and aThunkActionas aGlobalThunkAction, and this completes that distinction so those conversions are no longer possible.One recurring feature in this PR is adding fixmes: 3 of the 7 commits are small commits adding individual fixmes. Each of those is an item we'll need to return to and deal with before we can make a per-account state actually a different object from the global state, which is a necessary part of having a multi-account schema, #5006. But the key here is that these fixmes at individual spots allow us to then apply tighter type distinctions globally. That means that new code from unrelated ongoing development will naturally start respecting the new distinctions, which makes it productive to get the broad changes in sooner with a few fixmes and return to the fixmes later.
Coming next, we'll tighten the distinction between
GlobalStateandPerAccountStatein the same way as we did forGlobalDispatchandDispatch. Then, continuing as described last time: