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

Thrown errors swallowed when updating state with async function #607

Closed
mdtusz opened this issue Jul 20, 2021 · 1 comment · Fixed by #689
Closed

Thrown errors swallowed when updating state with async function #607

mdtusz opened this issue Jul 20, 2021 · 1 comment · Fixed by #689
Assignees
Labels
enhancement New feature or request

Comments

@mdtusz
Copy link

mdtusz commented Jul 20, 2021

With a read-write or write only atom that take an async function for the write argument (for using the "last" value), the errors seem to be swallowed.

const fooAtom = atom(0, async (get, set, update) => {
  const oldValue = get(fooAtom);
  const newValue = await update(oldValue);
  set(fooAtom, newValue);
}

// Then in some component...
const [foo, setFoo] = useAtom(fooAtom);

const onClick = () => {
  try {
    await setFoo(async (old) => await someFailableAsyncThing()) 
  } catch (e) {
    console.log("This will never be run");
  }
}

return <button onClick={onClick}>Foo</button>

As pointed out by @dai-shi, handling errors in this codepath is not yet implemented. From a DX point of view, it may make sense for the setState writer that is provided by useAtom to always re-throw any errors that are thrown by an update function, but there may be other consequences to how this is handled due to the mechanisms of Suspense.

@dai-shi
Copy link
Member

dai-shi commented Jul 20, 2021

Thanks for opening up the issue.

As I said, actually that was my original attempt in pre-v1, but had some difficulties in the implementation.

First, to be on same page, we support this.

it('can throw an error in write function', async () => {       
  const countAtom = atom(0)
  const errorAtom = atom(
    (get) => get(countAtom),
    () => {
      throw new Error()
    }                         
  )
                                      
  const Counter = () => {                       
    const [count, dispatch] = useAtom(errorAtom)
    const onClick = () => {
      try {                                                    
        dispatch()         
      } catch (e) {
        console.error(e)
      }
    }
    ...

In this case, we shouldn't require await for dispatch() because nothing is async from the developer's point of view.

Now, if it's async we want to catch with await.

  const countAtom = atom(0)
  const errorAtom = atom(
    (get) => get(countAtom),
    async () => {
      throw new Error()
    }                         
  )
                                      
  const Counter = () => {                       
    const [count, dispatch] = useAtom(errorAtom)
    const onClick = async () => {
      try {                                                    
        await dispatch()         
      } catch (e) {
        console.error(e)
      }
    }
    ...

Implementing this is already hard, but doable.

What if we have write chain:

  const countAtom = atom(0)
  const intermediateAtom = atom(
    (get) => get(countAtom),
    async () => {
      throw new Error()
    }                         
  )
  const errorAtom = atom(
    (get) => get(intermediateAtom),
    (get, set, update) => {
      set(intermediateAtom, update)
    }
  )

In this case, can the error be swallowed? Notice if we don't have async, it works.
We could put async/await in errorAtom write func, but it's not very nice to assume if intermediateAtom is async or not from Suspense perspective.


The other option, which is Suspense oriented, is to throw write error in render, so that you can catch it in ErrorBoundary. But, this doesn't sound right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants