[Tech] Remove the Global State #3827
Open
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
We already have this available globally on `window`, so we don't need it in the context provider as well
This also simplifies the logic for setting languages, as the component setting it now only has to call `setLanguage` & everything will update on its own
I'd recommend turning on the "Hide whitespace" option in GitHub's diff view for this commit
Note that storing this as an object is not a good idea, since `useShallow` checks for equality with `Object.is` (so when updating `helpItems`, we'd have to create a new `help` object, which would also cause re-renders for components only using `addHelpItem` / `removeHelpItem`)
CommandMC
force-pushed
the
tech/migrate-more-state
branch
from
e4ff8a7
to
4adc7e6
Compare
This was definitely the biggest change yet. Most of it was just taking a `runner` everywhere (to build a key to index into the record) This still has one issue (the library isn't refreshed after a game install/update finishes), it'll be resolved by future commits. Still, this should be a noticeable speed boost
CommandMC
force-pushed
the
tech/migrate-more-state
branch
from
4adc7e6
to
407c18a
Compare
I've chosen to inherit from the `ExperimentalFeatures` interface here to make picking out the values from the state easier. Otherwise, you'd need something like this for each one: ``` const enableNewDesign = useGlobalState(useShallow((state) => state .experimentalFeatures.enableNewDesign)) ``` which is rather verbose
This being a global function living on `window` was unnecessary as far as I could tell, and we can just implement it with a normal function being auto-called if `theme` updates
CommandMC
force-pushed
the
tech/migrate-more-state
branch
from
e4cf1e3
to
6ed21d2
Compare
This also modifies `InstallModal` to live on the Root & render based on only `installModalOptions`, like our other modals/dialogs do
CommandMC
force-pushed
the
tech/migrate-more-state
branch
from
b66b436
to
a3d156c
Compare
We can just do this on the one component calling this function This also introduces a local `refreshing` state for the Wine Manager, as the global state's is intended as a library refresh state
…deloadedLibrary` Quite a big commit, but splitting this up further isn't possible
CommandMC
force-pushed
the
tech/migrate-more-state
branch
from
a3d156c
to
556962a
Compare
CommandMC
changed the title
CommandMC
added
pr:testing
CommandMC
requested review from
a team,
arielj,
flavioislima,
Etaash-mathamsetty,
Nocccer and
imLinguin
and removed request for
a team
|
something seems to be wrong with the global state after uninstalling a game, when I try to close heroic it thinks it's still doing something and shows the
there's no way to make these changes more gradually? I know I always say the same but it's really hard to review 100 files changing, this changes a lot around the whole app, it's impossible for us to test/QA everything properly a nice thing that I liked about zustand was that it would allow us to move things to zustand gradually in different PRs with more focused scope to focus the test instead of changing everything, I'm always scared of changing that much I understand the commits are more granular, but everything in a single PR would block all the changes that are working because of issues in other places |
|
Part of why I was so granular with these commits is so we can take only a part of them and merge those. I'll create a new PR with some of the "easy" changes here, if that's easier |
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.
The GlobalState / ContextProvider is now replaced with a Zustand store, which improves the re-rendering issues dramatically
The commits here are quite bite-sized, so reviewing them one at a time is recommended. The commit messages also include details in case something was out of the ordinary when migrating that specific property
Known issues:
Use the following Checklist if you have changed something on the Backend or Frontend: