Skip to content
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

Merged
merged 7 commits into from
Jun 27, 2019

Conversation

drcmda
Copy link
Member

@drcmda drcmda commented Jun 25, 2019

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.

const foo = useStore(state => state.foo)
const bar = useStore(state => state.bar)

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.

import { shallowEqual } from 'zustand'

const { foo, bar } = useStore(state => ({ foo: state.foo, bar: state.bar }), shallowEqual)

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.

const fooSelector = useCallback(state => state.foo[props.id], [props.id])
const foo = useStore(fooSelector)

Can't live without redux-like reducers and action types?

const types = { increase: "INCREASE", decrease: "DECREASE" }

const reducer = (state, { type, by = 1 }) => {
  switch (type) {
    case types.increase: return { count: state.count + by }
    case types.decrease: return { count: state.count - by }
  }
}

const [useStore] = create(set => ({
  count: 0,
  dispatch: args => set(state => reducer(state, args)),
}))

const dispatch = useStore(state => state.dispatch)
dispatch({ type: types.increase, by: 2 })

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.

import { redux } from 'zustand/middleware'

const [useStore] = create(redux(reducer, initialState))

Devtools

import { devtools } from 'zustand/middleware'

// Usage with a plain action store, it will log actions as "setState"
const [useStore] = create(devtools(store))
// Usage with a redux store, it will log full action types
const [useStore] = create(devtools(redux(reducer, initialState)))

@drcmda drcmda closed this Jun 25, 2019
@drcmda drcmda reopened this Jun 25, 2019
@drcmda drcmda requested a review from JeremyRH June 25, 2019 10:13
@drcmda
Copy link
Member Author

drcmda commented Jun 25, 2019

Anything else we want to bring in?

@drcmda drcmda changed the title 1.x (optional equalityFn, error safe selectors) 1.x (optional equalityFn, error safe selectors, middleware: redux & devtools) Jun 25, 2019
```jsx
import { devtools } from 'zustand/middleware'

// Usage with a plain action store, it will log actions as "setState"

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?

Copy link
Member Author

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

@JeremyRH
Copy link
Contributor

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.

@drcmda
Copy link
Member Author

drcmda commented Jun 25, 2019

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' 

@JeremyRH
Copy link
Contributor

I turn off notifications for everything so there's no way to really ping me. I check my accounts somewhat often though.
Anyway, here is the list of changes I've made:

  • Removed shallowEqual.js.
  • Added new exported types: EqualityChecker, SubscribeOptions, StateCreator, Destroy.
  • Changed the function signature of: StateListener, Subscribe, UseStore.
  • useStore takes zero, one, or two arguments: (selector?, equalityFn?).
  • api.subscribe takes one or two arguments: (listener, options?). options being:
    { selector?, equalityFn?, initialSlice? }.

I also am using Object.is as the default equality function. There are minor differences from strict equality: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is

These are the three major differences from your changes:

  • Removed shallowEqual.js. Do we need to provide this or can the user get it from somewhere else?
  • Equality function defaults to Object.is instead of strict equality a === b.
  • api.subscribe:
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.

@drcmda
Copy link
Member Author

drcmda commented Jun 26, 2019

I turn off notifications for everything

wish i had your discipline ... i get nagged practically all day

Removed shallowEqual.js. Do we need to provide this or can the user get it from somewhere else?

👍 makes sense, let's cut it out

Equality function defaults to Object.is instead of strict equality a === b.

👍

api.subscribe:

Since the eq-fn isn't a primary concern for us in 1.x any longer i'd prefer going with the shorter signature:

api.subscribe(selector?, listener, equalityFn?)

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 sub(state => state.foo, foo => …). If you feel strongly about it, i would be ok with it either way - but yeah, these are my thoughts.

If you're fine with the last one, would you merge yours in?

@JeremyRH
Copy link
Contributor

There were two reasons for changing the signature of api.describe.

  1. Support passing a ref object so we:
    a. Don't need to call the selector more than once on initialization.
    b. Can update selector and equalityFn without resubscribing to the store or wrapping the functions.
  2. Reduce the complexity of the code. It took me a minute to remember what the code was doing after not looking at it for a couple weeks! This was mostly around the function passed to subscribe and how it calls a ref so we can change the ref value without resubscribing. I thought it was more simple to pass an object and the properties can be mutated without resubscribing.

Actually, looking at your changes, it seems equalityFn can't be changed after the first render. This can easily be fixed while still keeping the same signature you have. You just need to add a ref for equalityFn and pass a wrapper function that calls the ref.

@drcmda
Copy link
Member Author

drcmda commented Jun 26, 2019

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.

@JeremyRH
Copy link
Contributor

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?

@drcmda
Copy link
Member Author

drcmda commented Jun 27, 2019

I can do that, i will publish when your changes are in ...

@drcmda drcmda merged commit d46c758 into master Jun 27, 2019
@JeremyRH
Copy link
Contributor

JeremyRH commented Jun 27, 2019

@drcmda I've encountered an edge case around the subscribe function:
When a re-render occurs because of prop updates (not state updates), it causes useStore to be called but not subscribe. Both functions update their own copy of the state slice so calling one without the other causes the state slices to become out of sync.

I found this through writing tests. It can cause quite a few unexpected issues especially when using a custom equalityFn. Also defining the type signature for subscribe is difficult. Just look:

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 useStore and subscribe. We would have to add a 4th parameter to subscribe if we want to do that now.

@drcmda
Copy link
Member Author

drcmda commented Jun 27, 2019

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.

@JeremyRH
Copy link
Contributor

Thanks. And yeah, I'm just adding a few more tests and will try to merge it today.

@drcmda
Copy link
Member Author

drcmda commented Jun 28, 2019

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

  1. it says there that init is "required to not be 'undefined'" but selector can be undefined, no?
  2. i would like to wrap all selector calls into try/catch in order to forward errors to the component whose useStore calls will then produce undefined, rather than risking errors inside the selector or prepping all selectors with state && state.obj && state.obj.xyz. similar to how redux treats it. should we override the init function in there or switch back to useState?

@JeremyRH
Copy link
Contributor

@drcmda

  1. There is a problem with @types/react. It wont let you pass in a third argument with type T | undefined. I've changed the use of useReducer anyway so that isn't a problem anymore.
  2. It looks like react-redux's useSelector catches selector errors in the subscriber and will force the selector to be called again in useSelector. It does not return undefined but instead re-throws in useSelector.

@drcmda
Copy link
Member Author

drcmda commented Jun 28, 2019

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.

@drcmda
Copy link
Member Author

drcmda commented Jun 28, 2019

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.

@JeremyRH
Copy link
Contributor

JeremyRH commented Jun 28, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants