-
Notifications
You must be signed in to change notification settings - Fork 34
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
reset only subset of the state in withReset()
#160
base: main
Are you sure you want to change the base?
Conversation
Thanks @daboudii, there is already an extension I have to admit that I like your approach of using a function |
The |
@daboudii, yes what options do we have to do this in Would it help, if The reason why I'm pushing for the integration in the other feature is that we would not be limited to support a very specific use case ( |
Ok get it, I will have a look |
setInitial
in withCallState()
withReset()
@rainerhahnekamp let me know if it need something else |
withMethods((store): PublicMethods => { | ||
// workaround to TS excessive property check | ||
const methods = { | ||
resetState() { | ||
patchState(store, store._resetState.value); | ||
}, | ||
resetSlice(slice: string | string[]) { | ||
patchState(store, (state) => ({ |
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.
Nice work🎉🎉 @daboudii
It would be even better if we could just reset the part of the slice
in case of obj sometimes we don't want to reset the whole inner object but part of the object
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.
Something like resetSlice('address.city')
? If yes it will add even more complexity, is it really necessary ?
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.
Something like
resetSlice('address.city')
? If yes it will add even more complexity, is it really necessary ?
I think it will definitely add complexity but it will add much more value for project like ours and many others where data structures are much more complex
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 don't think I will add that, in this case just easier to patch your state with the value you which to change.
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 touched upon that in my review below #160 (review)
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.
Good job! I have a question about the design.
State changes would be better handled as functions within patchState. This ensures that patchState remains the sole entity modifying state and allows multiple update functions to run together as an “atomic update” of the internal Signal.
That being said, setResetState()
was already implemented in a way that doesn’t align with this approach. What if we provide a more flexible reset()
function that supports both full and partial state resets and is type-safe as well.
// reset the full state
patchState(store, reset())
// reset the partial state
patchState(store, reset(state => ({address: state.address, user: state.user}))
What do you say?
Yes it looks way better like that ! I'm starting the rewrite |
In case you need to re initialize your state