Closed matthieu-foucault closed 2 years ago
If we change the page with some client-side routing, CSN is back to true, and all is well again.
This case is intentional, if we render <Suspense>
on the client's first render there will be a mismatch between the client-generated HTML and the server-generated HTML (or at least React thinks so, despite there not being any actual change in the output).
If the query variables changes, and we stay on the same page (i.e. by having an action that changes the query string), then usePreloadedQuery will throw a promise at some point, but there is no Suspense to catch it, so it causes an error and the page fails to render
This is a really good point and definitely a bug that we should fix!
If the query variables changes, and we stay on the same page (i.e. by having an action that changes the query string), then usePreloadedQuery will throw a promise at some point, but there is no Suspense to catch it, so it causes an error and the page fails to render
Running into this issue as well when pushing a shallow route that updates a query param used in the page level query.
Verified this works for me https://github.com/bcgov/cas-cif/commit/5d496889871c3a9289068004110baf1404f9ac4e .
Given I was pushing a shallow route, I didn't expect the top level query to run.
Thanks for verifying that fix works, I'll fix this and push out a patch later today.
Thanks! Also, thanks @rrdelaney for maintaining this library 🙏 . It's helped us move very quickly on our project.
It's more a workaround than a fix, and suffers from the client/server render mismatch issue you noted (i.e. it triggers a warning to be printed in the console).
Noting that this may not be an issue when React 18 is released, as Suspense
will be available on the server
Alright, I was able to fix this in a relatively complicated manner. Just re-rendering on mount with the <Suspense>
wrapper led to some unnecessarily re-renders which caused some jank in on my site, but rendering with <Suspense>
always on the client-side ends up with React complaining about a markup mismatch. I ended up rendering the wrapper if one of the following are true:
Going to publish this in a new version soon.
Alright, created v0.6.0
and published on npm 😄 Let me know if it fixes your specific issue!
I have encountered a situation where the page state is lost when first time navigating with a shallow router push. I suspect that this is caused by the switch of render https://github.com/RevereCRE/relay-nextjs/blob/main/src/wired/component.tsx#L113-L123
The Component
is replaced with another instance for the first shallow router push, thus the local state of the original Component
is lost.
Since when the router is changed, new instance ofqueryVariables
will be returned.
https://github.com/RevereCRE/relay-nextjs/blob/9573b7aa9ca3237921c6d5cc26e8c267b36cc990/src/wired/component.tsx#L95-L97
The new instance will make the tracking always consider as changed. https://github.com/RevereCRE/relay-nextjs/blob/9573b7aa9ca3237921c6d5cc26e8c267b36cc990/src/wired/component.tsx#L73-L78
The approach of rendering different component trees after detecting query variable is changed will make the page component local state to be lost for the first time switch from server render tree to client render tree for shallow page navigation.
I think the library should provide an option to control the behaviour
useEffect
. (accepting the fact of the possibility of the jank caused by immediate re-render)@mortyccp I would recommend against navigating with a shallow router push, this library isn't designed to work with it. Is there a reason you must use shallow navigation?
Yes, my use case need to navigate with shallow route push.
@mortyccp this library is not designed to be used with shallow navigation. It lets Relay figure out when to fetch data or if it should be re-used from the cache. What is the problem you are trying to solve by using shallow navigation?
I have usage of having a swiper UI for the mobile view that will update the current URL to route-name/view1
or route-name/view2
and need to preserve the local state of the page. After finding seem the same instance of the page will be used and the state will be kept with or without the shallow navigation. So there is no need for support of shallow navigation atm. But the variable change detection bug still will affect this use case. Since even though the query variable is not updated, it's still detected as changed and trigger a component tree change and the local state is lost due to the old page instance is being replaced.
@mortyccp it seems like a fairly easy workaround would be to place the local state that needs persistence in either context or a Relay client schema extension. Those features are designed with your use-case in mind: preserving local state when the component hierarchy might change.
Not sure if I'll have time to make a small example, but I observed the following behaviour:
CSN
prop is false (meaning it won't be rendered inside aSuspense
component). Not sure if its an error from my code or if that's always the caseCSN
is back totrue
, and all is well again.usePreloadedQuery
will throw a promise at some point, but there is noSuspense
to catch it, so it causes an error and the page fails to render.I've only tested this with a development build so far, so maybe it's a different behaviour in production.