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

(suspense) - Suspense Exchange doesn't hold on to results and errors #1115

Closed
kitten opened this issue Nov 3, 2020 · 4 comments · Fixed by #1123
Closed

(suspense) - Suspense Exchange doesn't hold on to results and errors #1115

kitten opened this issue Nov 3, 2020 · 4 comments · Fixed by #1123
Assignees
Labels
bug 🐛 Oh no! A bug or unintented behaviour.

Comments

@kitten
Copy link
Member

kitten commented Nov 3, 2020

Originally reported by @StevenLangbroek

Summary

Steps to reproduce

  1. Create a client in suspense mode
  2. Add a query that always errors
  3. (optional) add suspenseExchange

See: https://github.com/StevenLangbroek/urql-suspense-loop-repro/tree/c8c88f42687e4f2c9ccc7839fa1526e13540769d

Expected behavior

The component suspends, the fallback renders, then on resolution of the query it displays the error.

Actual behavior

The component suspends, rerenders, then suspends again. This repeats indefinitely

Root Cause

The @urql/exchange-suspense package has not been kept up-to-date and we should consider moving suspense support back into mainline stable, and not experimental. @StevenLangbroek: Let me know if anything else is still causing trouble that we can address with this issue too, e.g. unexpected results after the latest changes on main.

The exchange should hold on to a result for one tick until the component re-renders at which point it can then reuse the result. I suspect that the double-subscription approach to initialise the effect subscription erases the result stored by the suspenseExchange before the component fully mounts, thus triggering suspense again.

@kitten kitten added the bug 🐛 Oh no! A bug or unintented behaviour. label Nov 3, 2020
@StevenLangbroek
Copy link
Contributor

Oh right forgot about that! Will do a couple of testruns 🙇

@kitten
Copy link
Member Author

kitten commented Nov 4, 2020

I believe the first step to investigating this which is a good stepping point is to write React Test Rendering unit tests for the suspense package, so I'll start with that and we'll go from there. This should show us the regression and prevent us from regressing in the future, so we can bring this package back from being marked as experimental or generally as a "bad idea" and officially support client-side suspense again.

@kitten
Copy link
Member Author

kitten commented Nov 6, 2020

With #1123 I believe I've found a solution that fixes all edge-cases related to client-side suspense and removes the requirement to add the suspenseExchange entirely. I've moved the suspense implementation away from @urql/core and the Client and instead to useQuery. It's noteworthy that if an uncached result is being fetched using Suspense on the client-side that urql will attempt to update the result on mount since it hasn't been cached. This however does not lead to a loop anymore.

@kitten
Copy link
Member Author

kitten commented Nov 6, 2020

Published in urql@1.11.0 and @urql/preact@1.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants