[v3] remove _handleSet from _TemporalState #154
Conversation
|
Ah, I see the issue. This is a really annoying part of zustand and I don't know the best practice. I'll make a discussion on the zustand repo to see what the problem is. I think the tests are failing due to a bug. We override the 
|
|
Made the discussion here: pmndrs/zustand#2305 |
|
Ah okay thank you for this! To make sure I understand, is the potential bug of directly modifying setState due to the fact that if multiple nested middlewares do this, setState will just be identical to the last middleware-defined set function instead of applying all nested middleware's set functionality? Or is it that if not all middleware's modify setState, set and setState would differ? I may also still be a little confused... |
Yup, exactly! If the user of the middleware expects that set and setState behave the same, then it's an issue. Looking at this more, I think we'll want to move forward with this change, but it'll have to be for v3 because it's a breaking change. It depends on setState rather than set. |
pstrassmann
changed the title
|
Ok sounds good! I updated the tests with your suggested fixes, which indeed stopped the tests from failing. If there's anything else you'd like me to do with this PR, just let me know. |
pstrassmann
changed the title
| @@ -700,6 +701,7 @@ describe('Middleware options', () => { | |||
| }, | |||
| 1000, | |||
| ); | |||
| store.setState = set; | |||
There was a problem hiding this comment.
Hey @pstrassmann, sorry for the delay. Life has been busy and I haven't made time for working on v3 of zundo; however, I'm here now after some good discussions in the zustand repo regarding authoring middleware.
Before merging this in, could you add another test case where store.setState = set isn't added and we expect .toHaveBeenCalled(0)? To put it in English, we should have a test case for zundo, there wrapTemporal fails to be called because we haven't modified the store setter.
src/index.ts
Outdated
| currentState, | ||
| ); | ||
| store.temporal.setState({ | ||
| ...store.temporal.getState(), |
There was a problem hiding this comment.
Why is this line needed here? Shouldn't we only need to be passing pastStates and futureStates here?
There was a problem hiding this comment.
I (mistakenly, I think) was under the impression that setState did not do the kind of auto-merging of state that set does, which is why I spread the other values here. I will remove this line.
There was a problem hiding this comment.
Gotcha, thanks. I think they both behave the same.
| it('should not call function if `store.setState` is not assigned to `set` (wrapTemporal)', () => { | ||
| global.console.info = vi.fn(); | ||
| const storeWithHandleSet = createVanillaStore({ | ||
| wrapTemporal: (config) => { | ||
| return (_set, get, store) => { | ||
| const set: typeof _set = (partial, replace) => { | ||
| console.info('handleSet called'); | ||
| _set(partial, replace); | ||
| }; | ||
| // intentionally commented out | ||
| // store.setState = set; | ||
| return config(set, get, store); | ||
| }; | ||
| }, | ||
| }); | ||
| const { increment } = storeWithHandleSet.getState(); | ||
| act(() => { | ||
| increment(); | ||
| }); | ||
| expect(console.info).toHaveBeenCalledTimes(0); | ||
| }); |
There was a problem hiding this comment.
Please feel free to suggest a better test description, or if you'd prefer the commented out lines to be removed
This PR removes the
_handleSetinternal property of_TemporalStatein favor of defininghandleSetwithintemporal(index.ts).This is beneficial from a complexity-reducing standpoint:
_handleSetno longer needs to be a member of the temporal state, and its functionality is easily discoverable and readable withintemporal.This change also allows us to remove the type assertion required for accessing private internal properties of
_TemporalStatewhen usinggetState(), which returns typeTemporalState.