vikejs / vike-react

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

add vike-react-apollo #125

Closed nitedani closed 1 month ago

brillout commented 2 months ago

:rocket:

Looking forward to merge :ok_hand:

You labeled it as draft so I guess it isn't ready for review yet, but let me know if you want a review.

nitedani commented 2 months ago

https://github.com/apollographql/apollo-client-nextjs/issues/325

It would be nice to use the official @apollo/client-react-streaming package instead of my StreamedHydration wrapper, but can't get it to work properly. https://github.com/apollographql/apollo-client-nextjs/tree/main/integration-test/vite-streaming/src

nitedani commented 2 months ago

Figured it out 😛

nitedani commented 2 months ago

The PR "works fine" right now, but it's not perfect. The last chunk is not injected to the stream, because the stream is already closed. https://github.com/vikejs/vike-react/pull/125/files#diff-faa397f7e357ac32094cc7b8c3506bb2f1142ccca3b8b22fb399c3c1fcc90853R18 It surprisingly doesn't cause any issues with my primitive examples.

There are 3 ways to solve it - hardest to easiest:

  1. https://github.com/brillout/react-streaming/issues/40
  2. add a hook to vike-react, allowing wrapping the stream returned by react-streaming with https://github.com/apollographql/apollo-client-nextjs/blob/e7a59cb26716a77bbfac659f435f89f5af8eff61/packages/client-react-streaming/src/stream-utils/createInjectionTransformStream.tsx#L18C17-L18C47
  3. use my custom StreamedHydration component instead of the official @apollo/client-react-streaming package, which has this issue - though I think this could cause some other issues in the future
  4. (bonus) leave it as-is, and fix it when we encounter an issue in the future🥹
brillout commented 2 months ago
  1. Allow passing async function to injectToStream brillout/react-streaming#40

If this is a reliable solution, then I'd be :+1: for doing this.

  1. add a hook to vike-react, allowing wrapping the stream returned by react-streaming with https://github.com/apollographql/apollo-client-nextjs/blob/e7a59cb26716a77bbfac659f435f89f5af8eff61/packages/client-react-streaming/src/stream-utils/createInjectionTransformStream.tsx#L18C17-L18C47

That could be nice as well :+1:. Some kind of generic hook enabling the user to manipulate the stream.

  1. (bonus) leave it as-is, and fix it when we encounter an issue in the future🥹

Let's see if we need go for the bonus :grin: (or 3.)

brillout commented 2 months ago
$ npm owner add nitedani vike-react-apollo
npm notice INFO: User nitedani invited to package vike-react-apollo successfully.
+ nitedani (vike-react-apollo)
brillout commented 2 months ago

Review done.

nitedani commented 2 months ago

ready

brillout commented 2 months ago

Reviewing done.

brillout commented 1 month ago

Idea:

// +Loading.js

export default {
  Layout: SomeLoadingComponent, // for both the Page and all Layouts
  Component: SomeOtherLoadingComponent
}

// Sets Loading.Component
export default SomeOtherLoadingComponent

WDYT?

nitedani commented 1 month ago

I like it because +LoadingComponent sounds weird, +Loading doesn't. But I don't know.

// Sets Loading.Component export default SomeOtherLoadingComponent

🧐

brillout commented 1 month ago

The way I see it is that's it's similar to overload function arguments.

setLoading(value) // What should this do?
setLoading({ component: value, layout: otherValue })

I'm inclined to make setLoading(value) be a shortcut for setting setLoading({ component: value }). Same for +Loading.js.

But maybe there is a better interface looming around the corner :thinking:

brillout commented 1 month ago

Actually, I'm not sure: what should be the default page transition animation? I think we should eventually add one instead of just showing a blank page. I'm asking because maybe we want to make export default SomeLoadingComponent apply to both components and layouts :thinking:

Or how about this: we only accept export default { component: Component, layout: Component } for now, and decide later what export default Component should do. So that we can proceed with this PR.

Unrelated: how about { components: Component, layouts: Component }? (Added s to the option names.)

brillout commented 1 month ago

Actually, I'm not sure: what should be the default page transition animation? I think we should eventually add one instead of just showing a blank page.

How about the "main Suspense approach" described at https://github.com/vikejs/vike-react/issues/117 (which I just updated)? So far I like this most. WDYT?

nitedani commented 1 month ago

export default { component: Component, layout: Component } for now

done

brillout commented 1 month ago

You're right, without s is better.

brillout commented 1 month ago

LGTM. Feel free to merge & release. We need to manually squash the PR in two commits: one for vike-react and one of vike-react-apollo.