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

Cancellation #10

Open
benjamingr opened this issue Feb 16, 2022 · 11 comments
Open

Cancellation #10

benjamingr opened this issue Feb 16, 2022 · 11 comments

Comments

@benjamingr
Copy link

The ask here is:

  • suspend accepts a first parameter that is an AbortSignal.
  • when the component is unmounted - abort the controller associated with the signal.

e.g.

function Post({ id, version }) {
  const data = suspend(async ({ signal }) => {
    const res = await fetch(`https://hacker-news.firebaseio.com/${version}/item/${id}.json`, { signal })
    return await res.json()
  }, [id, version])
  return (
    <div>
      {data.title} by {data.by}
    </div>
  )
}

Basically allowing aborting expensive async actions.

@joshuaellis
Copy link
Member

I think AbortSignal isn't fully supported in all browsers (safari) before promoting this pattern, are their polyfills // workarounds to resolve this?

@benjamingr
Copy link
Author

@joshuaellis sure there are polyfills though Safari supports it since Safari 11.1 (we are currently on Safari 15.2) so this should be safe. In any case there is a polyfill (the abort-controller npm package).

@drcmda
Copy link
Member

drcmda commented Feb 16, 2022

i think this would require suspend to be a hook because i can't call useEffect(() => () => ... otherwise?

another slight problem is the call signature because the parameters that suspend forwards are the cache keys (suspend((a, b, c) => ..., [a, b, c])), which then allows the function to be an external pure function. keys are spread.

so this would basically save bandwidth and avoid cpu/gpu overhead once the component unmounts? and the signal could be tested against right? so that if a browser doesn't have it, nothing happens and the thing that's loading will simply run its course wastefully.

@benjamingr
Copy link
Author

so this would basically save bandwidth and avoid cpu/gpu overhead once the component unmounts?

Right and also prevent cases where two suspends update the same data. e.g.

  • I have a suspend in a component
  • In it, I update state asynchronously making an API call using a setter I have from a parent component.
  • The parent component performs a render without me, I unmount - the state update did not finish yet.
  • The parent component re-renders the child with different props.
  • The component performs the suspend again, making an API call - this time it finishes quickly and the state is updated.
  • The original API call finishes - the original state update completes overriding the new one.
  • The user gets incorrect data.

@drcmda
Copy link
Member

drcmda commented Feb 16, 2022

about the call signature, maybe:

async function fn([a, b, c], { signal }) {
  ...
}

useSuspend(fn, [a, b, c])

so keys come out as they go in (as an array, no spread), second arg is an extensible object that could later be built out.

@benjamingr
Copy link
Author

benjamingr commented Feb 16, 2022

i think this would require suspend to be a hook because i can't call useEffect(() => () => ... otherwise?

That's pretty tricky, you would either need there to be a hook (even if suspend isn't) or you would need to wrap the component.

Here are three options:

function Post({ id, version }) {
  suspend.useCancellation(); // suspend below is now cancellation aware
  const data = suspend(async ({ signal }) => {
    const res = await fetch(`https://hacker-news.firebaseio.com/${version}/item/${id}.json`, { signal })
    return await res.json()
  }, [id, version])
  return (
    <div>
      {data.title} by {data.by}
    </div>
  )
}


function Post({ id, version }) {
  const data = useSuspend(async ({ signal }) => { // suspend is a hook
    const res = await fetch(`https://hacker-news.firebaseio.com/${version}/item/${id}.json`, { signal })
    return await res.json()
  }, [id, version])
  return (
    <div>
      {data.title} by {data.by}
    </div>
  )
}

// the component is wrapped
const Post = suspended(function Post({ id, version }) {
  const data = suspend(async ({ signal }) => {
    const res = await fetch(`https://hacker-news.firebaseio.com/${version}/item/${id}.json`, { signal })
    return await res.json()
  }, [id, version])
  return (
    <div>
      {data.title} by {data.by}
    </div>
  )
})

@drcmda
Copy link
Member

drcmda commented Feb 16, 2022

i would def prefer it to be a hook. i thought about it before and it used to be one. i just didn't use hooks until now and suspense can be randomly named, but thing like these would be an argument for doing it since it's more future proof.

@drcmda
Copy link
Member

drcmda commented Feb 16, 2022

would you make a pr for this @benjamingr we can review and find solutions when it comes up

@benjamingr
Copy link
Author

I'm a bit scared to ask how to run the tests :D ?

@drcmda
Copy link
Member

drcmda commented Feb 16, 2022

im glad you asked, because there aren't any. by all means ... add some :D

@alessiocancian
Copy link

i think this would require suspend to be a hook because i can't call useEffect(() => () => ... otherwise?

I'm using useLoader from react-three-fiber and having issues with useEffect, after hours of debugging I figured out it is caused by a suspend inside useLoader...
From what I understood currently there's no way to detect mount/unmount of a component which uses suspend or an hook calling it (like useLoader) before load is complete.
The only way to detect mount/unmount is to wrap it in another component with and using useEffect there but I don't know how useful it could be since even if we passed a param to the loading component useEffect with props as dependencies doesn't work either.

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

No branches or pull requests

4 participants