Urql: SSR + ConcurrentMode issues in 1.8.0
Describe the bug
I tried out updating Tripadvisor to 1.8.0 today and discovered an unfortunate bug that appears to only surface in ConcurrentMode (Real concurrent mode, not StrictMode)—It seems that while hydrating useQuery will occasionally return a fetching: true state with no data even with a fully populated SSR cache. This happens non-deterministically, about 50% of the time, and only when there are multiple queries on the page. Setting breakpoints prevents the failure case. As best I can tell, nothing is getting dispatched at all for the second query, however, they are both dispatched.
Steps to reproduce
https://codesandbox.io/s/cranky-bas-2nz2q
As noted, the behavior is nondeterministic, so it can take a few refreshes to see the error show up. A note on versions: React 16.9.0 is the latest semver'd release with access to the createRoot API, but I also validated that we see the same behavior with the experimental release corresponding to 16.12.0.
Expected behavior
Expectation here would be that the initial call to useQuery always returns available SSR data.
Actual behavior
Warning: Expected server HTML to contain a matching <div> in <div>. alongside the accompany nastiness that comes with React failing to hydrate.
Additional context
Interestingly, uncommenting the non-concurrent-mode hydration forces the concurrent mode one to work.
Guess it's worth noting that the reason I was so quick to try this out is that we were seeing some other funky (but far more minor) concurrent mode behavior with urql on 1.6.3 so after 1.8.0's release notes I was pretty excited for a potential solution!
marcusdarmstrong
All 31 comments
I suppose this may be a duplicate of #501, so it isn't specific to concurrent mode but instead what I'm seeing is that cached results (cacheExchange and ssrExchange) aren't being properly returned. I'll investigate and will hopefully be able to find a fix for the issue soon.
kitten
on 11 Jan 2020
Ah, interesting. I did some more limited testing around the non-concurrent portions of the site any didn’t notice anything, but must have been sampling bias or something.
Happy to provide some more test cases/etc. I tried my best at debugging it this afternoon myself but got pretty lost going through Wonka for the first time.
marcusdarmstrong
on 11 Jan 2020
@marcusdarmstrong This may fix #501, but I'm not sure yet whether your issue is identical. But you can try it out using the build from Pika CI, which is a preview package: https://github.com/FormidableLabs/urql/pull/503/checks?check_run_id=384443421
kitten
on 11 Jan 2020
Started testing as soon as I saw the PR! Looks great!
And even better, the promise of 1.8.0's fixes also took care of the issue over which I started the upgrade!
marcusdarmstrong
on 11 Jan 2020
I've just verified it with the CodeSandbox and it indeed seems to be fixed (for me the issue was reproducible rather consistently)
kitten
on 11 Jan 2020
The fix in #503 is now published with v1.8.1 :tada:
Thanks for opening such a quick issue!
kitten
on 11 Jan 2020
Thanks a ton for the fast response! (X2 today!)
marcusdarmstrong
on 11 Jan 2020
Unfortunately I'm still seeing issues with 1.8.2, these ones are ConcurrentMode specific:
https://codesandbox.io/s/clever-kapitsa-17x7n (Note that the sandbox also does function on 1.6.3)
Variable updates don't appear to actually make it to the exchange, and polling never occurs.
marcusdarmstrong
on 16 Jan 2020
The pollInterval is probably related (in this case) to the variables staying the same resulting in the same operationKey. This leads to the cache answerring hit to every query and returning said result. To force this away you can use requestPolicy: 'network-only' or cache-and-network
There were some errors in the codesandbox: started on correcting them: https://codesandbox.io/s/purple-hill-9wp5s
setArg always has the same reference so it should only be executed once. The query you are using doesn't accept any variables that's why it's not being sent again (key never changes).
It's quite odd after the first variable change it never seems to complete the operation anymore :/
JoviDeCroock
on 16 Jan 2020
Of course!
On Jan 16, 2020, at 4:21 AM, Jovi De Croock notifications@github.com wrote:
I'm going to split these up into two separate issues if that's okay with you?—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
marcusdarmstrong
on 16 Jan 2020
Fwiw, the setArg effect wasn't an attempt at a polling loop, just wanted a new variable set one time.
I hadn't swapped the query to a more correct one because the key doesn't seem to be dependant on parsed query information, only the passed variable set? (https://github.com/FormidableLabs/urql/blob/master/src/utils/request.ts#L25)
marcusdarmstrong
on 16 Jan 2020
Yes, in your case your key is always 1 because you set it to be that meaning your request will never change
JoviDeCroock
on 16 Jan 2020
That was the query key, not the request key—the request key should be computed on the hash of that plus the variables, since there is a variable object being provided. (Though that set was obviously unnecessary for this test—if you remove it from the query things behave the same way.)
marcusdarmstrong
on 16 Jan 2020
It seems that createRoot behaves differently as compared to Suspense and StrictMode. In StrictMode effects are run on a similar priority level as that of components but in createRoot it seems to transition to a separate execution model, trying to figure out what we can do about this.
JoviDeCroock
on 16 Jan 2020
@marcusdarmstrong it seems to occur when react-dom/react and scheduler are out of sync. So how I did it is I used react-dom, react and scheduler with the experimental flag. In the case of 16.10.2 that would be [email protected] I think this is mainly the reason they moved it to the experimental package instead.
JoviDeCroock
on 16 Jan 2020
16.10.2 + 0.16.1 should be consistent, right? I think that matches urql's requirements, but it doesn't seem to fix my repro case https://codesandbox.io/s/clever-kapitsa-17x7n
Edit: Just updated to the expermental tags in that sandbox and now it seems to at least work sometimes, but I'm seeing nondeterministic failures?
marcusdarmstrong
on 16 Jan 2020
@marcusdarmstrong would you mind trying with react-wonka: https://github.pika.dev/kitten/react-wonka/pr/11
JoviDeCroock
on 16 Jan 2020
Not seeing the symptoms with that!
marcusdarmstrong
on 16 Jan 2020
@marcusdarmstrong thank you for the swift responses, I'm going to try and look through the React codebase to see as to why this happens. Won't merge/publish that PR for the time being since the previous version seemed to work fine with Suspense/StrictMode. Hope you don't mind, want to get to the bottom of this
JoviDeCroock
on 16 Jan 2020
Sure thing; makes sense.
marcusdarmstrong
on 16 Jan 2020
This is a long shot, but while we have a fix for react-wonka, we'd also like to try out a use-subscription based solution that will not meddle with React timings at all 😅 https://github.com/FormidableLabs/urql/pull/514
As always, you can try this out using the Pika CI package: https://github.com/FormidableLabs/urql/pull/514/checks?check_run_id=398246009
On my end it seems to be behaving well although some tests will need to be updated.
kitten
on 17 Jan 2020
I'll give it a shot asap!
marcusdarmstrong
on 17 Jan 2020
Having some concurrent issues as well, not sure if it's related to the same thing, but I have a small concept app that works fine in legacy and blocking mode, but chokes up in concurrent mode.
Try things out with the various rendering modes. urql should be refetching on the submit of the form, and it does in legacy and blocking mode, but does nothing in concurrent.
Should I file a different issue?
chrstntdd
on 25 Jan 2020
@chrstntdd can you try out the build from #514 please? That should fix it we believe but it needs more testing
kitten
on 25 Jan 2020
I don't have it live yet, but I have a branch and have the same outcome - broken with concurrent mode. This is the error that gets logged if that helps.
I applied the build by checking out the feat/peek-query branch running yarn build && yarn postbuild, then doing a cp -r dist ../../path/to/project/node_modules/urql/dist, and running patch-package urql.
chrstntdd
on 25 Jan 2020
@chrstntdd that’s a separate issue related to us having accidentally dropped client-side suspense support. Our implementation of suspense is mainly there for SSR and when it’s used on the client-side, you’ll see undesired outcomes. We’re still looking into how we can retain this functionality, but it’s unlikely that we’ll continue supporting it as-is as this isn’t what Suspense is good at or for (meaning data loading / loading screens, rather than prerendering)
kitten
on 25 Jan 2020
Gotcha, so best course of action is to run with this for the browser client?
let client = createClient({
suspense: false,
// ...
})
It looks resolved with that branch. Works in all modes and I have it up running here.
Full changes
chrstntdd
on 25 Jan 2020
@chrstntdd Yep, if you're adding loading screens for the client-side then I'd set suspense to !process.browser (or typeof window !== 'object' depending on your bundler)
Great to hear that this works! We'll likely merge it then since it's also working well in the use-cases we've checked 🙌
(Btw you can install our PR changes from Pika CI which publishes temporary packages for each PR to save yourself the time to set up patch-package :))
kitten
on 26 Jan 2020
I'd regard this as fixed for now due to: #514, #523, and #521. A release with these fixes will follow soon
kitten
on 5 Feb 2020
Hiya, these additional fixes have now been released! Feel free to post an update whether they’re working well ♥️
kitten
on 13 Feb 2020
Will do, been held up on a few other items but probably will get around to trying an upgrade again in a week or two.
marcusdarmstrong
on 13 Feb 2020
Related issues
kitten
·
3Comments
TLadd
·
4Comments
davidhouweling
·
4Comments
narinluangrath
·
3Comments
kitten
·
4Comments