mobxjs / mobx-react-lite

Lightweight React bindings for MobX based on React 16.8 and Hooks
https://mobx.js.org/react-integration.html
MIT License
2.13k stars 91 forks source link

useLocalStore not updating with react-router-dom hooks #260

Closed probablykabari closed 4 years ago

probablykabari commented 4 years ago

The following example does not update the store when params are changed. Per the documentation, I would expect this to work or it should work if you wrap useParams in useAsObservableSource but that is not the case.

const Hello = () => {

  const params = useParams() // this is from react-router-dom
  const store = useLocalStore(source => ({
      get name() {
        source.name.toUpperCase()
      }
    }), params
  )

  return useObserver(() => (
    <div>
      <h1>Hello {store.name}!</h1>
    </div>
  ))
}

Example: https://stackblitz.com/edit/react-cbixnw

Versions

danielkcz commented 4 years ago

Well, perhaps adding a return to source.name.toUpperCase could be useful, what do you think? :)

probablykabari commented 4 years ago

@FredyC Oops, good catch! That example didn't exactly match what was happening in my application though. What I have is a store that initializes a class like so:

const StoreCreator = (name) => ({
  get upperName() {
    return name.toUpperCase()
  }
})

const Hello = () => {

  const params = useParams()
  const store = useLocalStore(
    source => new StoreCreator(source.name), 
    params
  )

  return useObserver(() => (
    <div>
      <h1>Hello {store.upperName}!</h1>
    </div>
  ))
}

This does not work. However, in doing this I realized the reason it doesn't work is that doing source => new StoreCreator(source.name) is essentially the same as de-structuring. So with that in mind, it makes sense why it does not update when the params change.

My question now would be, is there a way to do this without passing entire objects around like when using observable.box?

danielkcz commented 4 years ago

Yea, generally you always have to pass around observable objects or you will lose reactivity. The observable.box is useful for primitive values, but if there is no specific reason why not to pass a whole params in there, then do it.

Also why new? It's not like you have a real class there. It's just a factory function you can call like that. Or even directly like...

const store = useLocalStore(source => ({
  get upperName() {
    return source.name.toUpperCase()
  }
}), params)

Alternatively, useLocalStore(StoreCreator, params) would work as well.

probablykabari commented 4 years ago

It’s a very simplified example, but the string passed is actually like a sku number that gets parsed in the class to create/return another complex structure. I will change this functionality over to hooks eventually but it is a large project that I am migrating, so I’m trying to leave all the existing class-based stores as-is while updating all the components.

danielkcz commented 4 years ago

Well, if those stores are not reactive then a simple solution is basically to recreate that store when param changes. Instead of useLocalStore have a useRef to keep it and useEffect (that depends on that param) to recreate. I don't think there is a better way in such a scenario.

Brain2000 commented 4 years ago

useLocalStore returns a value from the hook useState, such as: return useState(() => observable(initializer(source)))[0] (this is simplified for brevity)

Since useState always returns the same reference on subsequent calls, it could be changed to: return useMemo(() => observable(initializer(source)), [current]) ("current" is the 2nd argument, so when that changes, useMemo will also re-calculate)

There is a line to consider just above the useState line: const source = useAsObservableSourceInternal(current, true)

Since useAsObservableSourceInternal( ) also calls useState instead of useMemo, it will also return the same reference every time, so that function will probably also need to be modified as well.

https://github.com/mobxjs/mobx-react-lite/blob/master/src/useLocalStore.ts https://github.com/mobxjs/mobx-react-lite/blob/a38cc5e21dc1b7f9beac33ec030acd4a1e44b7b4/src/useAsObservableSource.ts

danielkcz commented 4 years ago

Since useState always returns the same reference on subsequent calls, it could be changed to: return useMemo(() => observable(initializer(source)), [current])

NEVER do that, the useMemo is not guaranteed and may throw away its value randomly. Check React docs for that.

@kentcdodds Idea for blog post to mitigate this myth? :) Your https://kentcdodds.com/blog/usememo-and-usecallback doesn't mention it either.

Brain2000 commented 4 years ago

True, but even if it runs a 2nd time, I believe observable( ) will not change anything since isObservable( ) returns true.

danielkcz commented 4 years ago

@Brain2000 You are missing the point. If useMemo randomly forgets, you lose your state and it's recreated with initial values.

Brain2000 commented 4 years ago

Good point, if the object is statically defined that won't work. I'm used to always passing in an object already defined elsewhere, and setting the returned object back, similar to: tree.store = useLocalStore(() => tree.store)

I have to remember that not everyone does that. I suppose in my case I could switch it to a useCallback and just run plain old observable( ) on it.