-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POC: zustand for SelectedGroupStore #38775
POC: zustand for SelectedGroupStore #38775
Conversation
}), | ||
})); | ||
|
||
GroupStore.listen( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very gross
are we considering replacing our state management libraries with zustand? |
@priscilawebdev at the moment we're just comparing some different state management tools / considering contexts etc. to remove reflux or at the very least improve how we do cross-component state (eg. stores). |
set(({records: currentRecords, computed}) => { | ||
const {allSelected} = computed; | ||
|
||
const entries = ids | ||
.filter(id => currentRecords[id] === undefined) | ||
.map(id => [id, allSelected]); | ||
|
||
// Prune existing entries if requested | ||
const existingEntries = !prune | ||
? Object.entries(currentRecords) | ||
: Object.entries(currentRecords).filter(([id]) => ids.includes(id)); | ||
|
||
const records = { | ||
...Object.fromEntries(existingEntries), | ||
...Object.fromEntries(entries), | ||
}; | ||
|
||
return {records}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we're going to do zustand
you should also trying adding the immer
middleware which will get rid of the need for this ref manipulation to refresh state, it looks a lot closer the original store, and a more fair comparison from a code quality / simplicity standpoint. I know it's "more" since it's another concept but it helps avoid some of the problems with state mgmt in general, if we're going to switch it should realistically be to both.
eg.
immer<State>((set) => ({
...
add: (ids, prune) =>
set(state) => {
ids
.filter(id => !state.records.has(id))
.forEach(id => state.records.set(id, state.computed.allSelected));
}
...
})
Alternatively, maybe give it a try as a separate PR so we can see Zustand / Zustand + Immer / vs. when discussing it in TSC.
I'm looking forward to it! Please let me know if I can help with anything :) is React Query on our list? maybe I can work on a PoC too |
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
No description provided.