search

charkour / zundo

🍜 undo/redo middleware for zustand. <700 bytes
https://codesandbox.io/s/zundo-2dom9
MIT License
597 stars 19 forks source link

[v3] remove _handleSet from _TemporalState #154

Closed pstrassmann closed 6 months ago

pstrassmann commented 7 months ago

This PR removes the _handleSet internal property of _TemporalState in favor of defining handleSet within temporal (index.ts).

This is beneficial from a complexity-reducing standpoint: _handleSet no longer needs to be a member of the temporal state, and its functionality is easily discoverable and readable within temporal.

This change also allows us to remove the type assertion required for accessing private internal properties of _TemporalState when using getState(), which returns type TemporalState.

charkour commented 7 months ago

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 set callback in the wrapTemporal test but not the store.setState. If you add store.setState = set; right before returning the config, that should "fix" the tests. I think it's expected the middleware modify both, but I need to check with the zustand maintainers.

image
charkour commented 7 months ago

Made the discussion here: https://github.com/pmndrs/zustand/discussions/2305

pstrassmann commented 7 months ago

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...

charkour commented 7 months ago

Or is it that if not all middleware's modify setState, set and setState would differ?

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 commented 7 months ago

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.