-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add @requiresOptIn
directive
#943
Comments
That would be useful!
Or even deleting I suppose. |
I like this. I wonder if, given it's purely informational, it would be better for this to be provided via #300 rather than needing its own dedicated behaviour like |
Good question. I'm not really sure about introspection. I guess #300 would avoid adding any specific introspection handling there. In all cases, there's still the need to add it to the spec and reserve the name so that all tooling can agree on the semantics. That would allow all tooling working with SDL to support it. Tooling using introspection like GraphiQL would have to wait until #300 is added. |
I don't think we'd add it to the spec without it being introspectable, so it needs to be added to the introspection schema one way or another before it can be merged, in my opinion. I'm not keen for us to add an |
That was another question I had actually. I don't really see a use case for
Agreed 👍 |
Including experimental fields in introspection by default could result in clients accidentally using unstable experimental fields. Perhaps clients should be required to opt-in if they want to use experimental parts of the API to avoid such mistakes (although this makes #300 alone insufficient for solving this problem)? The GitHub API has pretty nice schema previews infrastructure for exposing experimental parts of their API that don't have the same stability guarantees as the rest of the API. It requires clients to opt into a preview by specifying an |
It almost feels like "deprecated" isn't really "deprecated" and more "don't include this by default because [...]" (deprecated/unstable/etc) |
@martinbonnin Would you like to present this to the GraphQL WG? Add yourself and a 20 minute agenda item to https://github.com/graphql/graphql-wg/blob/main/agendas/2022/2022-06-02.md (there's comments in that file instructing how to do so) or another convenient date. If you cannot make it to a GraphQL WG I'd be happy to raise it to the WG on your behalf, you can watch the discussion afterwards on YouTube. |
@benjie Sure thing! Pull request is there graphql/graphql-wg#964. |
* add support for @experimental See graphql/graphql-spec#943 * fix tests, remove debug * wip * add deprecated on input fields and arguments * add a validation error for deprecated arguments usage * added validation tests * ApolloExperimental -> Experimental * update apiDump
We've used Regarding |
Thoughts: Since Kotlin is the precedent for this behavior, consider they deprecated "experimental" in favor of optin Perhaps: directive @optIn(feature: String) Where elements with this directive are not exposed in introspection by default unless expressly asked for it. So adding:
Where the array of strings provided would need to match Open Qs from me:
|
If we're going the full Kotlin-like route, we could even think of adding directive usages to directive definitions: Specified directive: directive @optIn User schema for something like Github Deployment Previews: # optIn usage defines @experimentalDeploymentApi as an opt-in marker
directive @experimentalDeploymentApi @optIn
type Query {
deployment: Deployment @experimentalDeploymentApi
} This would require a change to the grammar:
To include/exclude introspection, we could pass a list of marker names:
That might take us a bit far but it'd allow more customization on the user side as the directive aren't limited to a single |
I've added a RFC at https://github.com/martinbonnin/graphql-wg/blob/opt-in-features/rfcs/OptInFeatures.md. @leebyron to answer your specific questions:
I would think so? Especially if we say A use case that comes to mind is a field that is super expensive to compute: type Stats {
aggregateAccrossTheWholeDb: Float @optIn(feature: "aggregates") @deprecated(reason: "use memoizedValue instead")
memoizedValue: Float
}
I think so... For deprecation, the edge case is arguments and input fields, right? I think it'd make sense to add the same kind of language for
Are there other cases? |
At Atlassian we mark fields in the schema with directive @beta(name: String!) on FIELD_DEFINITION
type Project {
"""
The team which owns the project
"""
team: Team @beta(name: "teams")
} That info is re-output in schema documentation (for GraphiQL and LSPs) by serialising it into the description of the field The team which owns the project
# Beta Field
This field is currently in a beta phase and may change without notice.
To use this field you must set a X-ExperimentalApi: AppTags HTTP header.
Use of this header indicates that they are opting into the experimental preview of this field.
If you do not set this header then request will be rejected outright.
Once the field moves out of the beta phase, then the header will no longer be required or checked. Similar to GitHub's However, we have a problem with our current situation. In Relay, fields are used in component fragments without knowing the query they're consumed by, and queries are run via We've ended up with a large list of features enabled for all queries, because we can't easily distinguish which query needs which experimental feature enabled. headers: {
'Content-Type': 'application/json',
'X-ExperimentalApi': [... all the features ...].join(","),
} Essentially invalidating the original desire for the We've had problems with teams unknowingly consuming exploratory fields because of this. Usage annotationWe're now looking to improve on this with a usage annotation directive @experimental(name: String!) on FIELD fragment Project_data on Project {
name
team @experimental(name: "teams")
} Which allows us to ensure you can't accidentally use a We have 2 possible ways to implement that
Tooling / IntrospectionNot having to abuse the description for this would be nice, so I like the idea of it being exposed via introspection in some way. We already use GraphiQL's "Request Headers" input to manually add the "X-ExperimentalApi" header, but buttons or checkboxes would be nicer BikesheddingWe've determined we want a different directives for the schema annotation than the query annotation, to keep them forward-safe if we determined we needed more than just a name. When using a header, the name was necessary to align the schema field with the usage. But when you annotate both sides, you no longer have the same need for a name. Although it may does continue to prevent people from slapping '@experimental' on every field in their query out of laziness BehaviourWe have a bit of an open question of what to do when someone doesn't opt-in on the query side. Right now, we fail the whole query (doesn't validate). But throwing an error or returning If we failed per-field, we might consider injecting errors (chaos-monkey style) to experimental fields to actively encourage them to mature, rather than allowing them to stagnate in the experimental state. |
Tangential to this particular experimental directive but the fact that the graphql spec does not allow "directives" to be listed on their usage sites (fields / types etc...) means that all the directive behavior has to be specified and built a priori into graphql engines. For example Without directives being available on fields / enums etc, I think it forces to much "implementation behavior" to HAVE to be in the graphql engine and not say in how the outside tooling might decide to treat it. In graphql-java we went beyond the spec and allowed introspection to show "applied directives" on the places they are used. (its opt in right now but we may make it default behavior because its super useful) So for example the documentation that @tomgasson mentioned could be "auto generated" by an external documentation tool because |
@bbakerman - 100% agree. This was one of the points of my presentation on GraphQL issues at the end of June meeting - to make introspection equivalent to SDL file, both have the same info; intro for tools, SDL for humans. You are right, all these headaches of accommodating new directives would be gone if we had this. |
@experimental
directive@requiresOptIn
directive
Hi. I want to voice my support for this proposal. I currently work at a company which maintains an API that is not only consumed by internal clients, but also by third party clients which we license our server software to. I am trying to convince our back end engineers to adopt a habit of deprecating APIs before removing them and a general hygiene of avoiding abrupt breaking changes. One of the arguments against this that I for see will be our inability to iterate quickly on APIs while enforcing a deprecation cycle. Until now we've benefitted a lot by the agility of only consuming our APIs internally, but only recently has the need for stable APIs arisen. Introducing an opt-in mechanism will allow my organization to iterate quickly on unstable APIs by dog fooding them with internal clients, while cautioning that third parties will be accepting risk by using them. Essentially the highly cohesive environment between client and server teams within our organization really benefits from a mechanism like opt-in, while still preserving API stability for clients who are more risk averse. I hope this perspective is valuable and encourages the adoption of this proposal. |
How does this address experimental types? For example, consider the example currently in the RFC: type Session {
id: ID!
title: String!
# [...]
startInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
endInstant: Instant @requiresOptIn(feature: "experimentalInstantApi")
} If More importantly, if a type itself is experimental, how should an API mark it as such to prevent others from accidentally using it from existing, stable fields? Consider an API with a type Query {
# [...]
node(id: ID!): Node
}
{
node(id: "foo") {
# Foo is experimental! We may want to rename or delete it before it
# stabilizes. Without opting in, users shouldn't be able to write a
# query that would be broken by those changes.
... on Foo {
id
}
}
} It seems to me like we might want to consider:
As an unrelated point, I don't see how the current proposal of the type __Schema {
# [...] other fields omitted for clarity
optInFeatures: [String!]!
} Edit: I see some of these concerns were brought up in graphql/graphql-wg#1006, but it doesn't seem like they were really addressed. |
@ccbrown lots of excellent points! Re: The reason I did not include them initially was that I copy/pasted the Would you include all types? directive @requiresOptIn(feature: String!) repeatable
on FIELD_DEFINITION
| ARGUMENT_DEFINITION
| INPUT_FIELD_DEFINITION
| ENUM_VALUE
# new values to support experimental types
| SCALAR
| OBJECT
| INTERFACE
| UNION
| ENUM
| INPUT_OBJECT The |
I would include all types. Theoretically, a server could be clever and infer the required features for all types other than object, but I think asking SDL authors to explicitly add the directive to all experimental types is better. It's clearer and more future-proof in the event that spec changes allowing interfaces and unions to implement interfaces are made. Once types are included, there should be a list of rules established for schemas, which can all be validated statically:
These rules would ensure that no matter what combination of features is used, the schema is always perfectly conformant. I'm in the process of building out a server implementation in ccbrown/api-fu#69 and this is the direction I'm currently taking. So far it seems to work well. |
You can read the rendered RFC at https://github.com/graphql/graphql-wg/blob/main/rfcs/OptInFeatures.md
Symmetrically to
@deprecated
, was it ever considered to add an@requiresOptIn
directive?@requiresOptIn
directives could be used to indicate that a given field hasn't reached stable status yet. It's usable for debug/development purpose but is also subject to breaking changes such as renaming and/or type change.Documentation and tooling could then use that information so that the client developer can make more informed decisions. In Kotlin, for an example, such fields could be annotated with an
Opt-in
annotation so that the caller has to explicitly enable them.The text was updated successfully, but these errors were encountered: