jotaijs / jotai-valtio

Jotai integration library for Valtio
MIT License
6 stars 1 forks source link

Bug: passing an object to mutableAtom is not registered to a provider. #9

Open alarbada opened 1 month ago

alarbada commented 1 month ago

Let me explain in code:

// this one updates as expected
const counterAtom = atom(0)
// this one aswell. The value is not shared when using Provider
const counterMutableAtom = mutableAtom(0)
// this one seems to ignore the Provider
const counterMutableObjectAtom = mutableAtom({ count: 0 })

function Counter() {
  const [count, setCount] = useAtom(counterAtom)
  const [countMutable] = useAtom(counterMutableAtom)
  const [countMutableObject] = useAtom(counterMutableObjectAtom)

  return (
    <>
      <div>
        <p>Count: {count}</p>
        <Button onClick={() => setCount((c) => c + 1)}>Increment</Button>
      </div>
      <div>
        <p>Count mutable: {countMutable.value}</p>
        <Button onClick={() => countMutable.value++}>Increment</Button>
      </div>
      <div>
        <p>Count mutable: {countMutableObject.value.count}</p>
        <Button onClick={() => countMutableObject.value.count++}>
          Increment
        </Button>
      </div>
    </>
  )
}

function ExampleOfBug() {
  return (
    <>
      <Counter></Counter>
      <Counter></Counter>
      <Provider>
        <Counter></Counter>
      </Provider>
    </>
  )
}

I've got no idea why, but using an object inside a mutableAtom makes it skip the Provider.

dai-shi commented 1 month ago

Nice catch!

I assume you are using Valtio v2.

The initialValue is shared across stores. We should deepClone when passing to atom().

https://github.com/jotaijs/jotai-valtio/blob/844b2193bcdd56f20a2fccfc5d244f70d458027e/src/mutableAtom.ts#L16

alarbada commented 1 month ago

Oh I'm so sorry, I did not include the versions of the packages.

The example is using "jotai": "^2.8.4" and "jotai-valtio": "^0.5.0". It's not using valtio directly.

I just installed valtio 1.13.2 as a peer dependency, and I can still reproduce the bug.

dai-shi commented 1 month ago

Hm, okay, yeah, it may happen with Valtio v1 because it's the same reference.

Do you think you can open a PR? Otherwise, I can do that.

import { deepClone } from 'valtio/utils'; // only available in Valtio v2

  const valueAtom = atom({ value: deepClone(initialValue) }); 
alarbada commented 1 month ago

pls open the pr, I tried but I couldn't find out how to import valtio v2 :S

dai-shi commented 1 month ago

No problem. Yeah, it's not very straightforward to upgrade to Valtio v2. Working on it.

dai-shi commented 1 month ago

Can you try #10? https://ci.codesandbox.io/status/jotaijs/jotai-valtio/pr/10 ☝️ Find "Local Install Instructions"


https://github.com/jotaijs/jotai-valtio/issues/9#issuecomment-2206254577 was wrong. It's a different line to apply deepClone.

alarbada commented 1 month ago

Nice, it works! Thanks man

I had to bun install valtio@2.0.0-rc.0 (I did not know about npm view valtio versions).

How stable do you think is version? Tbh I just need jotai Provider and mutableAtom for everything I want to do.

dai-shi commented 1 month ago

valtio v2-rc.0 should be pretty stable but I don't get much feedback. This PR should be fine. At least, it shouldn't cause any regressions.

alarbada commented 1 month ago

Then I'll happily use it. Thanks for your fast change!