-
Notifications
You must be signed in to change notification settings - Fork 990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support GraphQL Subscriptions in Apollo Client using SSE links #9009
Conversation
@dthyresson I released v6.0.3 probably just as you opened this one—changed it to next release patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dthyresson! A couple questions about the terminating link
@jtoar We discussed having some CI for realtime. Perhaps we do that in a different PR, but open to ideas since needs a serverful environment. |
I'll want to test with these upcoming Yoga and SSE changes: |
Will tackle double token request in SSE in another PR |
The CI smoke test fails due to a time out, but I ran them locally. Auth tests passed:
So, should be ok. |
Observable, | ||
} from '@apollo/client/core' | ||
import { print } from 'graphql' | ||
import { createClient, ClientOptions, Client } from 'graphql-sse' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stuff will mean these things will be included in the bundle for users even if they're not using sse (or deploying in a way sse is supported).
Suggestion:
I recently discovered that our RedwoodApolloProvider takes a linkFactory. I.e. you can append your own links to it.
// App.tsx
import { construcApolloLinkWithSse } from '@redwoodjs/realtime'
const App = () => (
<FatalErrorBoundary page={FatalErrorPage}>
<RedwoodProvider titleTemplate="%PageTitle | %AppTitle">
<AuthProvider>
<RedwoodApolloProvider
useAuth={useAuth}
logLevel="debug"
graphQLClientConfig={{
link: (rwLinks) => {
return constructApolloLinkWithSse(rwLinks)
},
}}
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see your point, but doesn't feel terribly elegant.
How far do we go? Do we keep making Providers for SSR and Subscriptions and with or without SSE?
I'd rather get this PR in so I can move forward and get feedback and then tackle:
- auth token optimization
- sse link / provider optimization
- sse with single connection mode and http2 dev support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of the suspense provider, its so that I don't break main for everyone - at some point the plan is to unify it (neither Apollo client nor I am ready for this just yet - we're still using the experimental nextjs client). Eventually there won't be a separate suspense provider
Yeah not elegant - think it needs more exploration - was just a suggestion to see if it gave you some ideas. We do have to be mindful of including things that aren't required by most people - but if you want to come back to this its up to you! I imagine this is the reason realtime was a separate package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I did some digging into the link and config and I see your point:
const App = () => (
<FatalErrorBoundary page={FatalErrorPage}>
<RedwoodProvider titleTemplate="%PageTitle | %AppTitle">
<AuthProvider>
<RedwoodApolloProvider
useAuth={useAuth}
logLevel="debug"
graphQLClientConfig={{
link: (rwLinks) => {
return constructApolloLinkWithSse(rwLinks)
},
}}
>
it would make sense to only have that link if realtime and subscriptions are enabled -- similar to have the sse plugin is added only if they are configured.
Maybe the realtime package has a RedwoodRealtimeApolloProvider
can adds in the terminating link?
Should probably label this as a breaking change, because of the change to the link name. Not an issue though 👌, just have to make sure we don't ship it in a minor! |
Sorry, I don't follow. a "change to the link name"? port type RedwoodApolloLinkName =
| 'withToken'
| 'authMiddleware'
| 'updateDataApolloLink'
| 'terminatingLink' The WAs this exposed to the user's config? I didn't realize that. |
…9009) This PR adds support for GraphQL SSE (Server Sent Events) in both the Redwood GraphQL Server and the Apollo Client web side. GraphQL SSE has two options: distinct connections mode and single connection mode. Yoga supports distinct connection mode out of the box and is the one configured in the PR. There is a known "gotcha" with distinct mode: * [Maximum open connections limit](https://developer.mozilla.org/en-US/docs/Web/HTTP/Connection_management_in_HTTP_1.x) (when not using http/2) * But we can work to support http2 in Fastify and also see how deploy providers handle http2 See also: https://github.com/enisdenjo/graphql-sse/blob/master/PROTOCOL.md#distinct-connections-mode Note: I have testing setting up single connection mode but this requires: * two graphql endpoints/servers at "graphql" for non subscriptions and "graphql/stream" for subs * some extra termination code as seen here: https://github.com/dotansimha/graphql-yoga/blob/main/examples/graphql-sse/src/app.ts We can revisit single connection mode configuration if in beta testing we see the distinct connection issue being significant. At least we now know how to configure and setup both modes. The PR: * Adds the graphql sse plugin to yoga if subscriptions are enabled * Adds a new SSELink to Apollo client following https://the-guild.dev/graphql/sse/recipes#with-apollo * In Apollo client, it needs to now use the SSELink for subs and the HTTPLink for other operations, so "directional link composition" is employed. See: https://www.apollographql.com/docs/react/api/link/introduction/#directional-composition * This is now the "terminatingLink" and the client uses one or the other depending on the operation * Fixes out-of-date realtime templates that now need to use the realtime package
Follow up to #9009 and #9205. #9009 increased the bundle size of Redwood apps: <table> <tr><th>stable (v6.3.3)</th><th>v6.4.0 RC</th><th>% increase</th></tr> <tr><td> - Rendered: 1.03MB (100.00%) - Gzip: 338.99KB - Brotli: 290.87KB </td><td> - Rendered: 1.46MB (100.00%) - Gzip: 461.74KB - Brotli: 394.16KB </td><td> - Rendered: 41.75% - Gzip: 36.21% - Brotli: 35.51% </td></tr> </table> I'm using [rollup-plugin-visualizer](https://www.npmjs.com/package/rollup-plugin-visualizer) on a newly-created Redwood app to get these numbers and treemaps. Here's links to the full treemaps: - [stable](https://github.com/redwoodjs/redwood/assets/32992335/41cf4204-10de-4b69-a07a-7d6b980ead31) - [v6.4.0 RC](https://github.com/redwoodjs/redwood/assets/32992335/f1a6a5bf-a260-45cb-8c97-f68b4fe0730d) We don't have a history of bundle size metrics, so it's hard to tell what the magnitude of this change is. But obviously in general, an increase isn't good, especially if it's for a feature that isn't used across the board. One solution was mentioned in this thread: #9009 (comment). It was to have users import the link, that way if they didn't, Vite wouldn't bundle it. The pushback there was more config for the user. I paired with @Josh-Walker-GM on a solution that doesn't involve any tradeoffs, except that it was work for us: we use Vite to remove the file from the bundle if realtime isn't configured. No bundle size increase if you're not using realtime, no config if you are! Note that we need to watch out for #9342 since it removes the option from the toml file that this solution uses. This was the main issue blocking v6.4.0, so now that it's resolved, we're ready to ship next week. **Update** This PR was failing the serve smoke test consistently. At first I thought it was because I didn't add the same logic to our webpack config, so I went ahead and did that but it was still failing. After a few dead ends, it turns out the logic I naively copied from the router to conditionally render the splash page has had a bug all along: https://github.com/redwoodjs/redwood/blob/699723910904d243ab153833b2d1a8e7494fcf40/packages/router/src/router.tsx#L120 You can see it easily by building and serving a Redwood app that doesn't have a home page: <img width="1186" alt="image" src="https://github.com/redwoodjs/redwood/assets/32992335/b4ee1369-af45-4887-aacf-3217212bdc68"> This is because this line is transpiled into: ```js var _splashPage = requireSplashPage(); // ... if (shouldShowSplash && typeof _splashPage.SplashPage !== "undefined") { ``` And you can't get away with `undefined.SplashPage`. The reason we didn't see it before is that hardly any user run `yarn rw serve` without a home page. Fixed by changing the module from a default `null` export to an empty object export: 591ee5a.
Follow up to #9009 and #9205. <table> <tr><th>stable (v6.3.3)</th><th>v6.4.0 RC</th><th>% increase</th></tr> <tr><td> - Rendered: 1.03MB (100.00%) - Gzip: 338.99KB - Brotli: 290.87KB </td><td> - Rendered: 1.46MB (100.00%) - Gzip: 461.74KB - Brotli: 394.16KB </td><td> - Rendered: 41.75% - Gzip: 36.21% - Brotli: 35.51% </td></tr> </table> I'm using [rollup-plugin-visualizer](https://www.npmjs.com/package/rollup-plugin-visualizer) on a newly-created Redwood app to get these numbers and treemaps. Here's links to the full treemaps: - [stable](https://github.com/redwoodjs/redwood/assets/32992335/41cf4204-10de-4b69-a07a-7d6b980ead31) - [v6.4.0 RC](https://github.com/redwoodjs/redwood/assets/32992335/f1a6a5bf-a260-45cb-8c97-f68b4fe0730d) We don't have a history of bundle size metrics, so it's hard to tell what the magnitude of this change is. But obviously in general, an increase isn't good, especially if it's for a feature that isn't used across the board. One solution was mentioned in this thread: #9009 (comment). It was to have users import the link, that way if they didn't, Vite wouldn't bundle it. The pushback there was more config for the user. I paired with @Josh-Walker-GM on a solution that doesn't involve any tradeoffs, except that it was work for us: we use Vite to remove the file from the bundle if realtime isn't configured. No bundle size increase if you're not using realtime, no config if you are! Note that we need to watch out for #9342 since it removes the option from the toml file that this solution uses. This was the main issue blocking v6.4.0, so now that it's resolved, we're ready to ship next week. **Update** This PR was failing the serve smoke test consistently. At first I thought it was because I didn't add the same logic to our webpack config, so I went ahead and did that but it was still failing. After a few dead ends, it turns out the logic I naively copied from the router to conditionally render the splash page has had a bug all along: https://github.com/redwoodjs/redwood/blob/699723910904d243ab153833b2d1a8e7494fcf40/packages/router/src/router.tsx#L120 You can see it easily by building and serving a Redwood app that doesn't have a home page: <img width="1186" alt="image" src="https://github.com/redwoodjs/redwood/assets/32992335/b4ee1369-af45-4887-aacf-3217212bdc68"> This is because this line is transpiled into: ```js var _splashPage = requireSplashPage(); // ... if (shouldShowSplash && typeof _splashPage.SplashPage !== "undefined") { ``` And you can't get away with `undefined.SplashPage`. The reason we didn't see it before is that hardly any user run `yarn rw serve` without a home page. Fixed by changing the module from a default `null` export to an empty object export: 591ee5a.
This PR adds support for GraphQL SSE (Server Sent Events) in both the Redwood GraphQL Server and the Apollo Client web side.
GraphQL SSE has two options: distinct connections mode and single connection mode. Yoga supports distinct connection mode out of the box and is the one configured in the PR. There is a known "gotcha" with distinct mode:
See also: https://github.com/enisdenjo/graphql-sse/blob/master/PROTOCOL.md#distinct-connections-mode
Note: I have testing setting up single connection mode but this requires:
We can revisit single connection mode configuration if in beta testing we see the distinct connection issue being significant. At least we now know how to configure and setup both modes.
The PR: