-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
1.x (optional equalityFn, error safe selectors, middleware: redux & devtools) #38
Conversation
Anything else we want to bring in? |
```jsx | ||
import { devtools } from 'zustand/middleware' | ||
|
||
// Usage with a plain action store, it will log actions as "setState" |
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.
Should we specify it actually logs in the Redux dev tools?
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 will rename the heading "redux devtools" just in case
Oh damn. I should have let you know I was working on something similar. I'll find the differences and try to figure out the best way to merge our work. |
oops, yeah im sorry - i didn't know : D are you active on twitter? i could ping you there next time. if you want you could join your changes in here. one more thing that's different, it copies package.json into the dist and switches it to private:false there, this allows us to push from /dist before import { devtools } from 'zustand/dist/middleware' now import { devtools } from 'zustand/middleware' |
I turn off notifications for everything so there's no way to really ping me. I check my accounts somewhat often though.
I also am using These are the three major differences from your changes:
api.subscribe(selector?, listener, equalityFn?)
// vs.
api.subscribe(listener, { selector?, equalityFn? }?) @drcmda let me know what you think. Here is my branch if you want to see the code. |
wish i had your discipline ... i get nagged practically all day
👍 makes sense, let's cut it out
👍
Since the eq-fn isn't a primary concern for us in 1.x any longer i'd prefer going with the shorter signature:
equalityFn as a 3rd param is ok imo b/c you only ever need it with a listener anyway, and that way it fits the useStore signature. most users would then just do If you're fine with the last one, would you merge yours in? |
There were two reasons for changing the signature of
Actually, looking at your changes, it seems |
hmmm 🤔 what you say makes sense to me, though don't you think DX could become a little more verbose? const foo = useStore(state => state.foo)
const unsub = subscribe(state => state.foo, {
selector: foo => ...
}) vs const foo = useStore(state => state.foo)
const unsub = subscribe(state => state.foo, foo => ...) but ultimately i trust your instinct, if you feel this will make it easier and faster - it's fine. |
Yeah, you're right. You've changed my mind haha. We should stick to the function signature without an options object. Should we merge this first and I'll add my changes after? |
I can do that, i will publish when your changes are in ... |
@drcmda I've encountered an edge case around the I found this through writing tests. It can cause quite a few unexpected issues especially when using a custom const subscribe: Subscribe<TState> = <StateSlice>(
selectorOrListener:
| StateSelector<TState, StateSlice>
| StateListener<StateSlice | undefined>,
listener?: StateListener<StateSlice | undefined>,
isEqualFn?: EqualityChecker<StateSlice>
) => {
const selector = listener
? (selectorOrListener as StateSelector<TState, StateSlice>)
: getState
const listener2 = (listener || selectorOrListener) as StateListener<
StateSlice | undefined
>
const equalityFn = (isEqualFn || Object.is) as EqualityChecker<StateSlice> This is just for the function signature and argument marshaling. No business logic yet. This is pretty much where I draw the line for a feature. Having a more terse syntax is not worth it in my opinion. If we use an options object instead of 2 optional functions, it makes it a lot more simple. Also we could have one source of truth for the state slice shared between |
i see. could you merge in your version? if we document it it's ok, we go with the slightly more verbose syntax then, it's just for subscribe anyway so no big deal. |
Thanks. And yeah, I'm just adding a few more tests and will try to merge it today. |
@JeremyRH one more thing i noticed, we're using useReducer now to keep the state, and it calls into the selector via the init function.
|
|
could we just define the selector as practically immune to errors, so that they will will faults in the component instead? const drawing = useStore(state => state.drawings[id]) // doesn't exist, but doesn't crash
drawing.doSomething() // -> should crash since hooks aren't ordered in sync with the render callstack, zombie children are a real problem (which we're starting to face right now. the only alternative would be safeguarding everything and that's quite an overhead. |
on second thought, maybe allowing errors on component rendering is fine? i guess it's only a problem when the selector can just fire out of sync with the render cycle. but if a component can render, it means its parent must be mounted, so it can't be a zombie child. |
Yeah, that's what I was thinking. If you look at this test I wrote, you'll see that errors in the selector (and equalityFn) are treated the same as any other errors that happen during the render phase. They can be handled with an error boundary. |
equality: #36
try/catch: #37
all tests seem to work, docs are updated
This would be a breaking change (major) since the useStore fn signature was slightly adjusted, removing dependencies (which are pretty much useless ever since we merged @JeremyRH's referened-selectors) in favour of an optional equality function.
It also adds basic middleware officially.
Selecting multiple state slices
zustand defaults to strict-equality (old === new) to detect changes, this is efficient for atomic state picks.
If you want to construct a single object with multiple state-picks inside, similar to redux's mapStateToProps, you can tell zustand that you want the object to be diffed shallowly.
Memoizing selectors
Selectors run on state changes, as well as when the component renders. If you give zustand a fixed reference it will only run on state changes, or when the selector changes. Don't worry about this, unless your selector is expensive.
Can't live without redux-like reducers and action types?
Or, just use our redux-middleware. It wires up your main-reducer, sets initial state, and adds a dispatch function to the state itself and the vanilla api. Try this example.
Devtools