-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Generalize Playground support to a simple "frontend page" plugin API #5206
Conversation
4fbd290
to
8df4b25
Compare
f405670
to
12a9e37
Compare
Apollo Server has graphql-playground built in, to the degree that the playground-html renderPlaygroundPage options are a top-level key in Apollo Server config. This has been very useful, but the theme of Apollo Server 3 is to keep its API lean and disentangled from other packages. Every single Apollo Server integration package has the same copy-and-paste code that threads playground options directly into the playground package. Additionally, graphql-playground has is retiring (see graphql/graphql-playground#1143) and merging back into GraphiQL. So it especially doesn't make sense for Apollo Server's API to continue to be defined by Playground. We could just switch back to GraphiQL like in Apollo Server 1, but instead, let's move UI configuration out of the top-level ApolloServer API and move it into our plugin system. And let's make sure that the per-web-framework integration packages don't need to care what UI system you're using. This PR adds a very very very simple static HTML serving API to our plugin system. The point of this system is to support plugins that serve a bit of HTML to load a full UI from CDN, like for Playground, GraphiQL, Explorer, etc, or to serve a splash page linking to other UIs. The point of this system is not to be a convenient way to add app-specific HTML to your app. If you want to do that, just use middleware in your web framework! The point is to make UI plugins that work out of the box with every Apollo Server integration. Because of that, the plugins can do very little: they can serve a single static HTML page. That's it! And you can only install one in your app. We add ApolloServerPluginUIGraphQLPlayground in core, as well as ApolloServerPluginUIDisabled. Like other plugins, if you don't set some UI plugin of your own and don't set the disabled plugin, Apollo Server will auto-install a plugin. In this case it installs the playground plugin though we may change that before 3.0.0. (The mechanism for implicitly installing ApolloServerPluginUIGraphQLPlayground is a bit more complex than for our other built-in plugins because we want user plugins to be able to override the UI, not just other built-in plugins. So ApolloServerPluginUIDisabled prevents ApolloServerPluginUIGraphQLPlayground from being installed at all, but other plugins defining renderUIPage get installed alongside ApolloServerPluginUIGraphQLPlayground and there's logic to make sure they take precedence.) We remove the `playground` constructor option (you can pass configuration options to ApolloServerPluginUIGraphQLPlayground instead), as well as three playground-related symbols exported from all the main packages. While we're at it, we simplify how we set up the Playground HTML: - There was some confusing logic in playground.ts where `defaultPlaygroundOptions.settings` listed some setting overrides... which were not actually used by default... only if you specified something like `playground: {settings: {}}`! We remove these and let the default settings in `@apollographql/graphql-playground-react` be the defaults *always* instead of *most of the time*. - Instead of trying to figure out the path to the GraphQL endpoint via a variety of different mechanisms (including "in the docs, tell you to edit the source to specify `endpoint` yourself), just leave `endpoint` empty by default. This makes the React app choose `location.href` for the endpoint, which should be fine! (Upgrade `@apollographql/graphql-playground-html` to not log a warning when `endpoint` isn't provided, and upgrade `@apollographql/graphql-playground-react` to preserve `code` query parameters on the endpoint so that the Azure Functions docs work automatically.) Also: - Tests that set environment variables (as some of the playground ones do) make me uncomfortable. Added `__testing__nodeEnv` to the ApolloServer and GraphQLOptions interfaces that can be used to test how `process.env.NODE_ENV` affects AS without actually changing the environment. - Inline fastifyApollo into fastify's ApolloServer. - I ended up rewriting a bunch of tests that used `request` to use `supertest` instead (though for some reason we access it via the symbol `request`). Many of these tests had been copied and pasted between packages but I did not consolidate them. Fixes #5159.
12a9e37
to
1e98e41
Compare
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.
You've done a great job with the implementation here and I don't see anything concerning. My comments are largely around naming and some nits on a couple patterns.
None of these concerns are things we couldn't merely adjust in a follow-up alpha release.
docs/source/integrations/plugins.md
Outdated
{ | ||
serverWillStart() { | ||
return { | ||
renderUIPage() { |
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.
The name of this just seems slightly askew from the patterns we've employed in the plugin API already. I don't know if I have a better suggestion.
renderUIPage() { | |
willRenderFrontend() { |
Doesn't strike me as better. Then I suppose my next question is, in case it wasn't clear from my hesitation to use UI
here is if Frontend
was just more expressive.
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'm not sure that will/did makes sense here because it's not just an observability hook noting that something happened — it actually does the rendering.
export function ApolloServerPluginUIGraphQLPlayground( | ||
options: ApolloServerPluginUIGraphQLPlaygroundOptions = Object.create(null), | ||
): ApolloServerPlugin { | ||
return require('./ui/graphqlPlayground').ApolloServerPluginUIGraphQLPlayground( |
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.
Wasn't immediately clear to me why this was a nested require
. Maybe a small comment?
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.
See the rest of the file including the large comment at the top.
// An internal-only flag used in the default UI plugin so that it can be | ||
// ignored if you install another UI plugin. | ||
__internal_installed_implicitly__?: boolean; |
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 a similar spirit to my previous thoughts about exposing things like __testing__
on the interfaces, would it be worth just not putting this here and @ts-ignore
-ing the small number of places we call it?
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 don't love ts-ignore, but I came up with a way to be basically typesafe here (using a type guard) while still keeping it off of the public interface.
@@ -79,6 +79,12 @@ export interface ApolloConfig { | |||
serverlessFramework: boolean; | |||
} | |||
|
|||
export interface ServeHtmlOptions { |
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.
Curious: Using ServeHtmlOptions
here versus Serve
HTML
Options
to align with HTML
capitalization/usage in other places because... this is public-facing? (I'm a fan of not capitalizing initialisms in camelCase, as you may know). Just trying to understand.
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'm not sure where we use capitalized HTML? But in any case, oops, this type was a leftover from an earlier version of this PR and should be removed.
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.
Ah I guess I wrote some internal prefersHTML
functions. I'll rename them.
@@ -20,7 +20,8 @@ export type InternalPluginId = | |||
| 'CacheControl' | |||
| 'SchemaReporting' | |||
| 'InlineTrace' | |||
| 'UsageReporting'; | |||
| 'UsageReporting' | |||
| 'UIDisabled'; |
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.
| 'UIDisabled'; | |
| 'FrontendDisabled'; |
Perhaps?
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 like UI -> Frontend!
Co-authored-by: Jesse Rosenberger <git@jro.cc>
Co-authored-by: Jesse Rosenberger <git@jro.cc>
Co-authored-by: Jesse Rosenberger <git@jro.cc>
Still pretty much typesafe, just not on the actual public API
Apollo Server has graphql-playground built in, to the degree that the
playground-html renderPlaygroundPage options are a top-level key in Apollo
Server config.
This has been very useful, but the theme of Apollo Server 3 is to keep its API
lean and disentangled from other packages. Every single Apollo Server
integration package has the same copy-and-paste code that threads playground
options directly into the playground package.
Additionally, graphql-playground has is retiring (see
graphql/graphql-playground#1143) and merging back into
GraphiQL. So it especially doesn't make sense for Apollo Server's API to
continue to be defined by Playground.
We could just switch back to GraphiQL like in Apollo Server 1, but instead,
let's move frontend configuration out of the top-level ApolloServer API and move it
into our plugin system. And let's make sure that the per-web-framework
integration packages don't need to care what frontend you're using.
This PR adds a very very very simple static HTML serving API to our plugin
system. The point of this system is to support plugins that serve a bit of HTML
to load a full frontend from CDN, like for Playground, GraphiQL, Explorer, etc, or to
serve a splash page linking to other UIs.
The point of this system is not to be a convenient way to add app-specific HTML
to your app. If you want to do that, just use middleware in your web framework!
The point is to make frontend plugins that work out of the box with every Apollo
Server integration. Because of that, the plugins can do very little: they can
serve a single static HTML page. That's it! And you can only install one in your
app.
We add ApolloServerPluginFrontendGraphQLPlayground in core, as well as
ApolloServerPluginFrontendDisabled. Like other plugins, if you don't set some frontend
plugin of your own and don't set the disabled plugin, Apollo Server will
auto-install a plugin. In this case it installs the playground plugin though we
may change that before 3.0.0.
(The mechanism for implicitly installing ApolloServerPluginFrontendGraphQLPlayground
is a bit more complex than for our other built-in plugins because we want user
plugins to be able to override the UI, not just other built-in plugins. So
ApolloServerPluginFrontendDisabled prevents ApolloServerPluginFrontendGraphQLPlayground from
being installed at all, but other plugins defining renderFrontend get installed
alongside ApolloServerPluginFrontendGraphQLPlayground and there's logic to make sure
they take precedence.)
We remove the
playground
constructor option (you can pass configurationoptions to ApolloServerPluginUIGraphQLPlayground instead), as well as three
playground-related symbols exported from all the main packages.
While we're at it, we simplify how we set up the Playground HTML:
defaultPlaygroundOptions.settings
listed some setting overrides... whichwere not actually used by default... only if you specified something like
playground: {settings: {}}
! We remove these and let the default settings in@apollographql/graphql-playground-react
be the defaults always instead ofmost of the time.
of different mechanisms (including "in the docs, tell you to edit the source
to specify
endpoint
yourself), just leaveendpoint
empty by default. Thismakes the React app choose
location.href
for the endpoint, which should befine! (Upgrade
@apollographql/graphql-playground-html
to not log a warningwhen
endpoint
isn't provided, and upgrade@apollographql/graphql-playground-react
to preservecode
query parameterson the endpoint so that the Azure Functions docs work automatically.)
Also:
me uncomfortable. Added
__testing__nodeEnv
to the ApolloServer andGraphQLOptions interfaces that can be used to test how
process.env.NODE_ENV
affects AS without actually changing the environment.
request
to usesupertest
instead (though for some reason we access it via the symbol
request
). Manyof these tests had been copied and pasted between packages but I did not
consolidate them.
Fixes #5159.