Skip to content
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

Merged
merged 27 commits into from
Aug 24, 2023

Conversation

dthyresson
Copy link
Contributor

@dthyresson dthyresson commented Aug 4, 2023

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:

@dthyresson dthyresson self-assigned this Aug 4, 2023
@dthyresson dthyresson added the release:feature This PR introduces a new feature label Aug 4, 2023
@dthyresson dthyresson added this to the v6.0.3 milestone Aug 4, 2023
@jtoar jtoar modified the milestones: v6.0.3, next-release-patch Aug 4, 2023
@jtoar
Copy link
Contributor

jtoar commented Aug 4, 2023

@dthyresson I released v6.0.3 probably just as you opened this one—changed it to next release patch

@dthyresson dthyresson marked this pull request as ready for review August 5, 2023 15:12
@dthyresson dthyresson requested a review from jtoar August 5, 2023 15:13
Copy link
Contributor

@jtoar jtoar left a 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

packages/web/src/apollo/index.tsx Outdated Show resolved Hide resolved
packages/web/src/apollo/index.tsx Outdated Show resolved Hide resolved
@dthyresson
Copy link
Contributor Author

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

@dthyresson
Copy link
Contributor Author

dthyresson commented Aug 11, 2023

@dthyresson
Copy link
Contributor Author

dthyresson commented Aug 16, 2023

Will tackle double token request in SSE in another PR

@dthyresson
Copy link
Contributor Author

The CI smoke test fails due to a time out, but I ran them locally. Auth tests passed:

[WebServer] api | 13:21:44 🌲 request completed 3ms
api | 13:21:44 🐛 Processing GraphQL Parameters done.
  ✓  3 [chromium] › rbacChecks.spec.ts:85:5 › RBAC: Admin user should be able to delete contacts (3.7s)

  4 passed (9.6s)

So, should be ok.

@dthyresson dthyresson requested review from dac09 and removed request for jtoar August 18, 2023 17:34
Observable,
} from '@apollo/client/core'
import { print } from 'graphql'
import { createClient, ClientOptions, Client } from 'graphql-sse'
Copy link
Collaborator

@dac09 dac09 Aug 21, 2023

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) 
            },
          }}
        >

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

@dthyresson dthyresson Aug 21, 2023

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?

@dthyresson dthyresson enabled auto-merge (squash) August 24, 2023 15:30
@dac09
Copy link
Collaborator

dac09 commented Aug 24, 2023

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!

@dthyresson
Copy link
Contributor Author

dthyresson commented Aug 24, 2023

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 'terminatingLink' vs httpLink before?

WAs this exposed to the user's config? I didn't realize that.

@dthyresson dthyresson merged commit a13174a into redwoodjs:main Aug 24, 2023
27 checks passed
@dthyresson dthyresson added the release:breaking This PR is a breaking change label Aug 25, 2023
@dthyresson dthyresson modified the milestones: next-release-patch, v7.0.0 Aug 25, 2023
@dthyresson dthyresson removed the release:breaking This PR is a breaking change label Sep 8, 2023
@dthyresson dthyresson modified the milestones: v7.0.0, next-release Sep 8, 2023
@jtoar jtoar modified the milestones: next-release, v6.3.0, v7.0.0 Sep 16, 2023
jtoar pushed a commit that referenced this pull request Oct 8, 2023
…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
@jtoar jtoar modified the milestones: next-release, v6.4.0 Oct 10, 2023
jtoar added a commit that referenced this pull request Nov 13, 2023
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.
jtoar added a commit that referenced this pull request Nov 13, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants