WiredComponent should always render a Suspense on the client

[matthieu-foucault]

Not sure if I'll have time to make a small example, but I observed the following behaviour:

• When rendering a page for the first time on the client, the CSN prop is false (meaning it won't be rendered inside a Suspense component). Not sure if its an error from my code or if that's always the case
• Now there are two scenarios after this:
  • If we change the page with some client-side routing, CSN is back to true, and all is well again.
  • 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.

I've only tested this with a development build so far, so maybe it's a different behaviour in production.

[rrdelaney]

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!

[alecrobins]

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 bcgov/cas-cif@5d49688 .

Given I was pushing a shallow route, I didn't expect the top level query to run.

[rrdelaney]

Thanks for verifying that fix works, I'll fix this and push out a patch later today.

[alecrobins]

Thanks! Also, thanks @rrdelaney for maintaining this library 🙏 . It's helped us move very quickly on our project.

[matthieu-foucault]

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

[rrdelaney]

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:

1. The page is being rendered for the first time and it's the client-side
2. The query variables have changed since the initial render

Going to publish this in a new version soon.

[rrdelaney]

Alright, created v0.6.0 and published on npm 😄 Let me know if it fixes your specific issue!

[mortyccp]

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.

relay-nextjs/src/wired/component.tsx

Lines 95 to 97 in 9573b7a

	const queryVariables = useMemo(() => {
	return (opts.variablesFromContext ?? defaultVariablesFromContext)(router);
	}, [router]);

The new instance will make the tracking always consider as changed.

relay-nextjs/src/wired/component.tsx

Lines 73 to 78 in 9573b7a

	useEffect(() => {
	setVarsChanged((current) => {
	if (current === 'pending') return false;
	return true;
	});
	}, [queryVariables]);

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

1. Immediately re-render with the client-side tree with useEffect. (accepting the fact of the possibility of the jank caused by immediate re-render)
2. Re-render with the client-side tree after query variables changed (accepting the page local state to be lost when first re-render trigger on a shallow route change)
3. Always render the client-side tree (accepting the react mismatch warning)

[rrdelaney]

@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?

[mortyccp]

Yes, my use case need to navigate with shallow route push.

[rrdelaney]

@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?

[mortyccp]

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.

[rrdelaney]

@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.