atomWithRefresh example from the docs doesn't work

[a10nik]

Bug Description

Example in the docs suggests we pass the refresh function like this:

  const [posts, refreshPosts] = useAtom(postsAtom);
...
      <button type="button" onClick={refreshPosts}>

It results in a React's click event being implicitly passed to the setter. The setter checks if arguments are empty, and they are not. It then assumes we're trying to set something into the atom rather than refresh it. But the write function is not passed therefore it does nothing.

It looks like we need to change the order of conditions here like this so that it only cares about its arguments if it is writeable

An alternative is just to update the docs and tell people to wrap refresh functions into lambdas. However, this won't stop people from trying and getting confused. Especially since even Typescript allows to coerce function types like that and doesn't complain.

Another alternative would be to split atomWithRefresh into 2 functions: one writeable and the other read-only. It would save us 1 tricky if-else and prevent bugs like these in the future. However, it would introduce unnecessary breaking changes and make it harder for the users to remember.

Reproduction Link

https://stackblitz.com/edit/vitejs-vite-yeqmd4n4?file=src%2FApp.tsx

[dai-shi]

Nice catch. I think it's the doc issue.

<button type="button" onClick={() => refreshPosts()}>

We shouldn't pass event anyways.

[dai-shi]

@a10nik Can you send a PR to fix the docs instead of #2996?

[a10nik]

I can, but I'm still not convinced that it should only be about the docs:

1. I think that the arguments/no arguments distinction only makes sense for writable refreshable atoms, not for fetch-only ones.
2. I don't think there is much point in the current noop behaviour when there is no write function and some arguments are passed. I cannot think of an example where a guaranteed noop is a desired behaviour for a setter and it seems to always indicate a bug in the user's code. At the very least we should make it print a dev warning.

export function atomWithRefresh<Value, Args extends unknown[], Result>(
  read: Read<Value, Args, Result>,
  write?: Write<Value, Args, Result>,
) {
  const refreshAtom = atom(0)
  if (import.meta.env?.MODE !== 'production') {
    refreshAtom.debugPrivate = true
  }
  return atom(
    (get, options) => {
      get(refreshAtom)
      return read(get, options as never)
    },
    (get, set, ...args: Args) => {
      if (args.length === 0) {
        set(refreshAtom, (c) => c + 1)
      } else if (write) {
        return write(get, set, ...args)
      } else {
        // Here we are doing nothing even though refresh is called. Let's print a warning
      }
    },
  )
}

4. People tend to write callbacks like this in React all the time, even without the docs telling them to. It's a common practice in React, they just assume the extra arguments will be ignored. See 1st example here, the event is ignored too. I think the fact that someone, even knowing how it works, still subconcsiously wrote the doc like that indicates how common the practice is and therefore how common the mistake is going to be.
5. One cannot typecheck against that
6. I believe the fetch-only case is much more common among atomWithRefresh's usages. I don't think we should make those users who only have fetch-only ones aware of some behaviour specific to writeable ones.
7. There are no other places in jotai's API I'm aware of that check the arguments length like that. So it's not like the users expect us to

If you're concerned about introducing breaking changes I'm all for fixing the docs AND adding the warning now and only changing the noop behaviour in the next major version, in a separate branch. But simply to leave it as it is now feels wrong to me. Unless I'm missing something, in this case please do tell!

[dai-shi]

// Here we are doing nothing even though refresh is called. Let's print a warning

Yeah, let's throw an error in DEV mode.

if (import.meta.env?.MODE !== 'production') {
  throw new Error(...)
}

Can you modify #2996 for this?