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

Infinite recursion in selector #10

Closed
dy opened this issue Apr 16, 2019 · 10 comments
Closed

Infinite recursion in selector #10

dy opened this issue Apr 16, 2019 · 10 comments

Comments

@dy
Copy link

dy commented Apr 16, 2019

Now these awesome storage hooks as side-effect solve the task of singleton-hook, like hookleton, so that makes them useful for eg. history/routing.

Would be super-cool also to incorporate "use-effect"-purpose into the standard selector function as

let [query, setQuery] = useState({ param1, param2 })
let { value, error, loading } = useZustandStore(state => ({
value: state.fetch(query),
...state
}, [...query])


// ... in zustand-store
fetch = async (query) => {
set({loading: true, error: null, success: null})
// ..await something
set({loading: false, success, error})
return success
}

Now that selector creates nasty recusion, making that method of querying data useless.
What would be the right solution here? That is not elegant to put fetching logic in a separate useEffect(fetchingLogic, [...query]).

@dy
Copy link
Author

dy commented Apr 16, 2019

That does not even allow to make sync selectors.

useStore(state => state.query(), [])

// in query
const [useStore] = create((set, get) => ({
query: () => (state.set({loading: true}), state.set({loading: false}))
}))

this raises unstoppable infinite recursion, so that even empty deps [] don't guard.

Expectation:

  1. Gate deps [] should run selectorFn just once for the deps set, even if previous selector is not finished.
  2. Calling (indirectly) set from selectorFn should not trigger component rerender instantly to cause recursion.

@drcmda
Copy link
Member

drcmda commented Apr 16, 2019

Not sure if calling into state inside the selector is a good idea. The selector is fired on every state change. Now the selector calling set again is pretty wild as well, shouldn’t you just fetch query and call it inside useEffect?

@dy
Copy link
Author

dy commented Apr 17, 2019

Well that’s just a footgun, I shouldn’t I guess, but that would be useful combination of useEffect, useState, store and hookleton/global state in one. Now that is not as much useful for async methods, like fetching - they oftentimes come with store.

@drcmda
Copy link
Member

drcmda commented Apr 24, 2019

i mean, not opposed, but do you have an idea how we could solve this?

@dy
Copy link
Author

dy commented Apr 24, 2019

I used to set “busy” flag to a function via https://ghub.io/icicle, but more generally that is sort of “mutex”.
Recently I’ve postponed setting to ‘useEffect’, so querying happened in different hook.
But I think some code rearrangement could help, should have a look at the source more thoroughly.

@JeremyRH
Copy link
Contributor

JeremyRH commented Jun 1, 2019

The selector isn’t meant to be used for effects. Supporting this would be very difficult or impossible with the current design. The selector is called when the state is updated to determine if the selected state has changed. This leads to an infinite loop if you set state in the selector: setState()-> selector() -> setState()-> selector() etc.

@JeremyRH JeremyRH closed this as completed Jun 1, 2019
@drcmda
Copy link
Member

drcmda commented Jun 1, 2019

@JeremyRH i think in redux there was something that made it possible. Could setState set a flag and if it's set it will postpone the update?

let active = false
let queue = []
let raf
function setState(...args) {
  if (active) 
    // push arguments to queue
    queue.push(...args)
    // cancel old frame and create new
    if (raf) cancelRequestFrame(raf)
    raf = requestAnimationFrame(setState)
  }
  active = true
  // merge queued up state and empty queue
  queue.forEach(...set state)
  queue = []
  // merge current state (if any)
  ... set args state
  ... call listeners
  active = false
}

@JeremyRH
Copy link
Contributor

JeremyRH commented Jun 1, 2019

@drcmda You would end up with inconsistencies in state because setState is no longer guaranteed to set state synchronously:

set({ count: get().count + 1 }) // get() wont return queued state

You can solve this by always setting state even if active is true. The problem is the listeners are called.

Also asynchronous effects would not be blocked by this:

const useStore = create(set => ({
  something: {},
  async selectorWithEffect() {
    const something = await waitForSomething()
    set({ something })
    return something
  }
}))
function App() {
  const promise = useStore(s => s.selectorWithEffect())
}

The callstack would be something like:

etc.
...async
(active = false)
waitForSomething
selectorWithEffect
set (active = true)
...async
(active = false)
waitForSomething
selectorWithEffect
set (active = true)
...async
waitForSomething
selectorWithEffect
useStore

@JeremyRH
Copy link
Contributor

JeremyRH commented Jun 1, 2019

My thoughts:

I don't think effects inside the selector is a good pattern to follow. useStore is more like useState or useReducer, not useEffect. We have to call the selector when state changes to get a new slice. Stopping set from calling the listeners is patchwork. There could be other effects outside of set that we have no control over. It seems like supporting this would lead to more problems than it solves. If we had the ability to only run the selector once, or when the dependencies list changes, it might be worth implementing. But I don't think this is a good idea right now.

@drcmda
Copy link
Member

drcmda commented Jun 2, 2019

Makes sense. It was just something i thought remembered about redux. But they probably came to the same conclusion: https://stackoverflow.com/questions/36730793/can-i-dispatch-an-action-in-reducer

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

No branches or pull requests

3 participants