jotaijs / jotai-valtio

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

feat: add api for returning a valtio proxyState from a jotai atom derived from another atom value #5

Closed dmaskasky closed 11 months ago

dmaskasky commented 11 months ago

mutableAtom

mutableAtom wraps a value in a self-aware Valtio proxy. You can make changes to it in the same way you would to a normal js-object.

API Signature

function mutableAtom(value: Value, options?: Options<Value>): Atom<{ value: Value}>

Parameters

Example

Count value is stored under the value property.

const countProxyAtom = mutableAtom(0)

function IncrementButton() {
  const countProxy = useAtomValue(countProxyAtom)
  return <button onClick={() => countProxy.value++}>+</button>
}

Options

Options include proxyFn which can be proxy or a custom function.

type ProxyFn<Value> = (obj: { value: Value }) => ({ value: Value })

type Options<Value> = {
  proxyFn: ProxyFn<Value>
}

Caution on Mutating Proxies

Be careful not to mutate the proxy directly in the atom's read function or during render in React components. Doing so might trigger an infinite render loop.

const countProxyAtom = mutableAtom(0)

atom(
  (get) => {
    const countProxy = get(countProxyAtom)
    countProxy.value++ // This will cause an infinite loop
  },
  (get, set) => {
    const countProxy = get(countProxyAtom)
    countProxy.value++ // This is fine
  }
)
codesandbox-ci[bot] commented 11 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 500401d79e481ea4d0c8e5d5029e8fcf6c919cc8:

Sandbox Source
React Configuration
React TypeScript Configuration
dai-shi commented 11 months ago

Thanks for opening up a PR.

API design questions:

  1. atomWithProxyEffect(initialValue) would be easier to implement than withProxyEffect(atomToSync). Do you want to attach a capability to an existing atom? It makes a little bit complicated (too flexible), which is another concern.
  2. Is it an effect? It sounds to me it allows "mutation". If something called withProxyEffect is possible, I think withEffect the same capability without proxy sounds possible. But, is there such a thing?

Overall, one of my questions is if this falls into the "impure" category of Jotai mental model.

dmaskasky commented 11 months ago

atomWithProxyEffect(initialValue) would be easier to implement

It wouldn't be too much work to convert the code to (1). 😅 I like this idea too, it prevents developers from passing a writable atom to the utility that doesn't accept args of the form [Value].

I think withEffect the same capability without proxy sounds possible.

It might be possible, not sure. But I wouldn't be in favor of reverse engineering Valtio around atomEffect. The two are not similar. withProxyEffect must return a useable mutable proxy from first read.

my questions is if this falls into the "impure" category of Jotai mental model.

The utility does allow Valtio-like imperative mutations similar to $state returned by useProxy from valtio/utils.

When sync: true, the utility tries to perform these mutations synchronously.

That is...

get(countAtom) // 0
$count.value++
get(countAtom) // 1

sync: false updates with a delay of Promise.resolve().then.

I should note that sync:true is synchronous only after onMount has fired, so initial read is still treated the same as sync: false. But synchronous imperative updates on mount is an anti-pattern so this is not a case we should handle.

(i.e. the following use case should not be supported)

const countAtom = atom(0)
const proxyCountAtom = withProxyEffect(countAtom, { sync: true })

function SomeComponent() {
  const $count = useAtomValue(proxyCountAtom)
  $count.value++
  const count = useAtomValue(countAtom)
  expect(count).toBe(1) // it will be 0 because count is updated on next microtask
    ...
}
dai-shi commented 11 months ago

I think withEffect the same capability without proxy sounds possible.

I think I misunderstood something. Proxies are required for mutation. So, I think it's different from atom-effect's effectFn. I would probably call it atomWithMutability or mutableAtom.

When sync: true, the utility tries to perform these mutations synchronously.

It feels to me that sync: true should be default and not configurable.

$count.value++

This should be prohibited anyway in render. It should follow Jotai's principle.

dmaskasky commented 11 months ago

atomWithMutability or mutableAtom

😮 Oh I like those choices. I wasn't too fond of withProxyEffect, it was just a placeholder.

It feels to me that sync: true should be default and not configurable.

Yes, we should lock it to sync: true so that it behaves the same as synchronous updates in Jotai.

This should be prohibited anyway in render. It should follow Jotai's principle.

Agreed, but mutableAtom does allow for some strange patterns when used inside other atoms. For instance, mutating in the read is possible which causes an infinite loop. I wish there was some way to prevent this.

const countProxy = mutableAtom(0);

const readOnlyAtom = atom(get => {
  const $count = get(countProxy)
  $count.value++ // infinite loop ⚠️
});
dai-shi commented 11 months ago

I wish there was some way to prevent this.

I wish it too. For now, I think it should be clearly documented.

dmaskasky commented 11 months ago

Requested changes complete.

  1. rename withProxyEffect to mutableAtom
  2. mutableAtom accepts a value, not an atom
  3. test that mutableAtom can accept function as value
  4. add warning to readme for infinite loop when proxy is mutated on atom read or component render
  5. remove sync option and hard-code sync config to true
dmaskasky commented 11 months ago

What should happen if proxyState were to be read / written to after the atom has unmounted?

dmaskasky commented 11 months ago

I would generally avoid leaking general get and set to avoid misusing in our code. Instead, I'd pass around store and proxyRef (unless combined) objects, and setValue function. Functionality-wise, it should be the same, so basically just a suggestion for coding preference.

I refactored storeAtom.onMount and merged it with proxyRef. Does this resolve your concern?

createProxyState is used in:

  1. storeAtom write function (uses get and set)
    • for store.onMount
  2. proxyEffectBaseAtom read function (uses get and setSelf)
    • for synchronous first read
  3. proxy wrapper get and set functions (uses get and setSelf)
    • for synchronous remount

I think it is better to centralize this logic, but let me know if you prefer I break this up.

dai-shi commented 11 months ago

What should happen if proxyState were to be read / written to after the atom has unmounted?

BTW, reading/writing atoms without mounting is totally valid operations.

import { createStore, atom } from 'jotai';

const store = createStore();
const countAtom = atom(0);
store.set(countAtom, 1);
console.log(store.get(countAtom));

This is true, even after mounting and unmounting.

dai-shi commented 11 months ago

Hm, actually this is interesting. My gut feeling is we don't need to re-create proxyState. Just create once and subscribe on creation. We never unsubscribe. Everything would be garbage collected if the atom is garbage collected. I think it will clean things up and become less hacky. (I might be missing something though...)

dmaskasky commented 11 months ago

BTW, reading/writing atoms without mounting is totally valid operations.


it('should allow updating even when component has not mounted', async () => {
  const store = createStore()
  const countAtom = atom({ value: 0 })
  const mutableCountAtom = makeMutableAtom(countAtom)
  await act(async () => {
    const countProxy = store.get(mutableCountAtom)
    countProxy.value++
  })
  expect(store.get(countAtom).value).toBe(1) // true 😮
})```
dmaskasky commented 11 months ago

Great suggestions!

My gut feeling is we don't need to re-create proxyState. Just create once and subscribe on creation. We never unsubscribe. Everything would be garbage collected if the atom is garbage collected.

Done.

I would generally avoid leaking general get and set to avoid misusing in our code. Instead, I'd pass around store and proxyRef (unless combined) objects, and setValue function. Functionality-wise, it should be the same, so basically just a suggestion for coding preference.

Done.

dai-shi commented 11 months ago

Here you go: https://github.com/dmaskasky/jotai-valtio/pull/1

dai-shi commented 11 months ago

By the way, I would simply use expect. Is assert simply for a shorthand?

I mean the goal is to drop minimalistic-assert completely.

Please also resolve conflicts.

dmaskasky commented 11 months ago

I mean the goal is to drop minimalistic-assert completely.

I chose minimalistic-assert because another package we are depending on already uses it. Plus it is a devDependency so it won't increase the bundle size. I'm happy to write the logic for this too as it is simply

function assert(value: boolean, message?: string) {
  if (!value) {
    throw new Error(message ?? 'Assertion failed')
  }
}

By the way, I would simply use expect. Is assert simply for a shorthand?

@testing-library has a strange behavior where waitFor can run an expect that passes twice. This causes expect.assertions to fail.

dai-shi commented 11 months ago

https://github.com/testing-library has a strange behavior where waitFor can run an expect that passes twice. This causes expect.assertions to fail.

Hmm, curious how it fails.

I'm happy to write the logic for this too as it is simply

Yeah, that's much easier for me to read. Let's do that unless we find a solution to the above issue.

dmaskasky commented 11 months ago

Hmm, curious how it fails.

It runs in a setTimeout loop, but an extra run can occur when expect is successful. The internet told me to use assert instead.

Yeah, that's much easier for me to read. Let's do that unless we find a solution to the above issue.

Done.

dai-shi commented 11 months ago

The diff looks weird. For example, it shouldn't show changes in .github.

dmaskasky commented 11 months ago

The diff looks weird. For example, it shouldn't show changes in .github.

Hopefully this is resolved now. I was in rebase hell for the past hour.