[graphql-js] require opt-in to allow defer/stream #12
Replies: 6 comments 23 replies
-
|
@glasser The |
Beta Was this translation helpful? Give feedback.
-
|
@IvanGoncharov Keeping it in sync for the SDL first flow is as easy as doing the following. Unfortunately, there is no way to get the Directive AST with graphql.js 😢 import { GraphQLDeferDirective, GraphQLStreamDirective } from "graphql"
import { makeExecuteableSchema } from "@graphql-tools/schema";
import { astFromDirective } from "@graphql-tools/utils";
const deferDirectiveAST = astFromDirective(GraphQLDeferDirective)
const streamDirectiveAST = astFromDirective(GraphQLStreamDirective)
const schema = makeExecuteableSchema({
typeDefs: [...userTypeDefs, deferDirectiveAST, streamDirectiveAST],
resolvers,
});The current feature flag implementation also does not allow enabling only |
Beta Was this translation helpful? Give feedback.
-
|
It might also be important to get the opinion of the other major schema builder libraries. @hayes for GiraphQL |
Beta Was this translation helpful? Give feedback.
-
|
That's so verbose @n1ru4l 👀 If we think about it, there is 2 things at play. The schema and what that means to the executor.
and call it a day, everything else is up to the
execute({schema, document, enableIncrementalDelivery: true})
means, the schema can be defined within the current eco system, and then the executor just opts in. And everyone is happy, and the world makes sense again. Absolutely no reason to over complicate things. |
Beta Was this translation helpful? Give feedback.
-
|
I still think that the @maraisr What you are proposing is to have both the developer provide the directives to the schema, and then also have the developer provide the The example I showed is explicitly for usage with |
Beta Was this translation helpful? Give feedback.
-
|
The spec clearly states that its a SHOULD not a MUST, so there is cases where you want the directive but not the server counter part. To blindly infer the presence of the behavior purely based on the presence of a directive is a smell, and one we should avoid.
Correct. We can never enforce this behavior on users, as it 100% requires a client that also supports the transport medium. SO until there is an eco-system buy-in, and is majorly used, both need to be provided.
With all due respect, I don't think we should optimize the api of graphql-js to suffice your library. You can 100% if youd like during your, what, |
Beta Was this translation helpful? Give feedback.
-
|
Also @tgriesser @Sytten for Nexus. |
Beta Was this translation helpful? Give feedback.
-
|
Since the original discussion got a bit heated, emotional, and too vague (picking certain aspects and ignoring others), I tried to gather all mentioned ideas and concerns, outline the pros and cons, while also adding my personal conclusion. The problemThe core question that started the debate is the following:
Use-case: apollo-server wants to support the latest graphql-js versions as a (peer-)dependency as soon as possible, without the need to support all the new features supported by it, like incremental delivery. Proposed Solutions1. Feature Flag within graphql-js GraphQLSchemaWith those requirements in mind, the idea came up to disable The default value of const schema = new GraphQLSchema({
enableDeferStream: true,
...remainingParams,
});This flag modifies the const schema = new GraphQLSchema({
enableDeferStream: true,
// incrementalDelivery: true will result in adding the stream directive anyways
// -> It is impossible to only allow defer or stream this way.
directive: specifiedDirectives.filter((directive) => directive.name !== “stream”),
});Pros:
Cons:
2. Assert the GraphQL schema with codetype ServerArgs = {
schema: GraphQLSchema
port?: number
hostname?: string
}
function createServer({ schema, ...rest }: ServerArgs) {
if (schema.getDirectives().some(directive => directive.name === "defer" || directive.name === "stream")) {
throw new Error("This framework does not support defer and stream.")
}
/// ... other logic
createHTTPServer()
}Pros:
Cons:
3. Filter the GraphQL schema with codetype ServerArgs = {
schema: GraphQLSchema
port?: number
hostname?: string
}
function createServer({ schema: inputSchema, ...rest }: ServerArgs) {
const schemaConfig = inputSchema.toConfig()
schemaConfig.directives = directives.filter(directive => directive.name !== "defer" && directive.name !== "stream"))
const schema = new GraphQLSchema({ ...schemaConfig })
/// ... other logic
createHTTPServer()
}Pros:
Cons:
4. Feature Flag for
|
Beta Was this translation helpful? Give feedback.
-
|
@n1ru4l thank you for writing this up, I think it's very helpful. I think one additional thing that needs to be considered is that introspection is the primary way for a client to know if their server supports defer/stream. The defer/stream directives should not be in the schema if the server does not support them. The spec draft currently states
Separating the concerns of adding the directives and enabling their execution could make it very easy to create a server that's not compliant with the spec. |
Beta Was this translation helpful? Give feedback.
-
|
The defer/stream directives currently only SHOULD be respected, might a server want to disable for an entire request based on high load or some other reason? A flag on execute could help with that theoretical use case, although certainly could be checked as each directive is evaluated… |
Beta Was this translation helpful? Give feedback.
-
|
Also think it’s important that we avoid flag on schema that skips default defer/stream and disables execution, as I will probably want to implement a custom stream directive that has a custom argument to include info on when individual streams end, for example, respected by graphql-executor, not (yet) by graphql-JS. When the argument is disabled, graphql executor and graphql JS should give identical response, but with current setup, graphql js would not honor stream at all. i don’t think this is just about my single use case, library, I think it’s about the equation between whether the defer/stream directives are introspectable = whether the DEFAULT defer/stream directives are introspectable |
Beta Was this translation helpful? Give feedback.
-
|
Regarding disabling defer and stream on a request basis, there are also these questions I raised some time ago: graphql/graphql-over-http#167 It might make sense that the server does not use incremental delivery in case the user does not accept multipart responses. |
Beta Was this translation helpful? Give feedback.
-
|
This was a really great summary @n1ru4l; thanks for putting the time into this! TL;DR In my opinion:
Reasoning follows... From my perspective, I'm not sure that we necessarily ever want to enable Separately, subscriptions currently require a different method ( With regards to schema design (and specifically what is exposed to users to indicate the capabilities of the server), I think it makes sense to allow |
Beta Was this translation helpful? Give feedback.
-
|
@IvanGoncharov @n1ru4l @saihaj @maraisr @glasser I added this to the agenda of the next GraphQL-JS WG meeting on Jan 26. |
Beta Was this translation helpful? Give feedback.
-
|
The result from the Working Group discussion:
|
Beta Was this translation helpful? Give feedback.
-
|
As one of the original people asking about this, that works for me. Two clarifying questions:
|
Beta Was this translation helpful? Give feedback.
-
|
Yes, a schema can only contain defer or only stream. I also think that defer will reach broader adoption than stream. Ideally a server would verify a schema does not contain the defer and the stream directive if it does not support incremental deivery or raises an error if execute does not return a Promise. Additionally it would be an additional layer of security if the AsyncGenerator from execute is lazy aka only starts setting up the underlying event listeners upon the first .next() call on the iterable. That could avoid potential memory leaks. We would consider adding an executeAsync function that rejects in case the execution results in an AsyncGenerator instead of a promise if really necessary. |
Beta Was this translation helpful? Give feedback.
-
|
I updated the defer-stream branch with the agreed changes. I also added a tutorial to the new docs site with an example of how to pass the directives to the schema. |
Beta Was this translation helpful? Give feedback.
-
|
@robrichard how does one get push rights to graphql-js repo 🤔 |
Beta Was this translation helpful? Give feedback.
-
|
What would be the correct way for users who use SDL schemas to enable directive @defer(if: Boolean, label: String) on FRAGMENT_SPREAD | INLINE_FRAGMENT
directive @stream(if: Boolean, label: String, initialCount: Int = 0) on FIELD
# rest of schema
type Query {
}As mentioned previously, this leaves room for error, as the directive definition should match exactly what is in the spec and what graphql-js is expecting. There is a private function import {GraphQLDeferDirective, GraphQLStreamDirective, printDirective} from 'graphql';
const schema = `
${printDirective(GraphQLDeferDirective)}
${printDirective(GraphQLStreamDirective)}
# rest of schema
type Query {
}
`; |
Beta Was this translation helpful? Give feedback.
-
|
The directives probably doesn't need to be provided in the template literal, as if you're using graphql-js, they can be provided directly to the buildSchema methods 🤔 so both as-code and as-sdl folk, can both supply the directives in the same way. 🤝 |
Beta Was this translation helpful? Give feedback.
-
|
@maraisr can you show me an example? |
Beta Was this translation helpful? Give feedback.
-
|
So; new GraphQLSchema({
directives: [GraphQLDeferDirective],
query: graphql`type Query { ... }`
});Granted my as-code looks a little more like import { buildGraphQLSchema } from "gqtx";
import { GraphQLDeferDirective, specifiedDirectives } from "graphql";
import { mutation, query } from "./types";
const directives = [...specifiedDirectives, GraphQLDeferDirective];
export const makeSchema = () => {
const schema = buildGraphQLSchema({
directives,
query,
mutation,
});
schema._enableDeferStream = true;
return schema;
}; |
Beta Was this translation helpful? Give feedback.
-
Context
asked by @glasser graphql/graphql-js#2848 (comment)
Decision
Consequences
Beta Was this translation helpful? Give feedback.
All reactions