vikejs / vike-react

🔨 React integration for Vike
https://vike.dev/vike-react
MIT License
105 stars 16 forks source link

make vike-react-query stream optional #62

Closed phonzammi closed 10 months ago

phonzammi commented 10 months ago

This is just a bit improvement for vike-react-query. Don't force user to use stream, maybe some users just want to use react query without streaming.

brillout commented 10 months ago

use react query without streaming.

I don't think that works. Have you tried?

nitedani commented 10 months ago

Without streaming enabled, the issue is that Suspense won't work server-side(technically it renders the fallback, and it recovers on client, but throws an error for some reason), so the withFallback or useSuspenseQuery won't work correctly. The user would need to use useQuery, and that would work without errors.

vike-react_renderer_onRenderClient.js?v=67f609b5:15312 Uncaught Error: The server did not finish this Suspense boundary: The server used "renderToString" which does not support Suspense. If you intended for this Suspense boundary to render the fallback content on the server consider throwing an Error somewhere within the Suspense boundary. If you intended to have the server wait for the suspended component please switch to "renderToPipeableStream" which supports Suspense on the server

For withFallback I think it's possible to fix, and then useSuspenseQuery can be used inside withFallback, something like:

  const ComponentWithFallback = (componentProps: T) => {

    const hasStreaming = ...
    const [mounted, setMounted] = useState(!!hasStreaming)
    useEffect(() => {
      setMounted(true)
    }, [])

    if (!mounted) {
      return typeof Fallback === 'function' ? <Fallback {...componentProps} /> : Fallback
    }

But we can't modify the useSuspenseQuery code itself(unless we re-export it from vike-react-query), so it would only work without errors if used within withFallback.

So, while the stream: false doesn't work without errors for the current examples/react-query example, it would work if useQuery is used intead of withFallback and useSuspenseQuery. Hydration is not possible without streaming, because for it to work, the Suspense boundaries need to be awaited on the server(requires streaming).

brillout commented 10 months ago

I stand to be corrected, but I think it's fine if we require streaming for vike-react-query.

phonzammi commented 10 months ago

I don't think that works. Have you tried?

It is work, yes I've tried it.

So if users don't want to use streaming, then they don't need to use withFallback & useSuspenseQuery, they can still use every other features of react-query.

for the current examples/react-query example

I've also add stream: true in the pages/config.h.ts, so this example still works

brillout commented 10 months ago

Again, I'm not sure making streaming optional is worth the added complexity.

That said, supporting ssr: false in a seamless fashion could be nice. But, instead, we can simply tell users to use React Query directly without vike-react-query.

So, unless I'm missing something, I'm inclined to think that going in that direction isn't worth it.

phonzammi commented 10 months ago

I'm closing this PR then. FYI, I think we should update react-streaming for vike-react-query package. If for some reason users set config stream: false then they won't get an error

brillout commented 10 months ago

I think it's fine; it isn't a blocker.