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

Expand @deprecated to Objects #997

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fotoetienne
Copy link
Contributor

@fotoetienne fotoetienne commented Oct 20, 2022

Problem

Take as a motivating example:

type Query {
  animals: [Animal]
}

interface Animal {
  name: String
}

type Dog implements Animal {
  name: String
}

type Baiji implements Animal {
  name: String
}

The Baiji type corresponds to an Animal that is no longer known to exist, and the server will no longer return this type.
We would like to delete the code for this type and eventually remove from the schema, but first clients must remove all references of this type from their queries. Currently, there is no good way to indicate to clients that they should no longer spread on this type in their queries.

Solution

Allow @deprecated on objects. Marking as deprecated indicates to clients that this type will no longer be returned from the server. This can indicate to client build tooling that references to this object should be removed from client queries.

type Baiji implements Animal @deprecated {
  name: String
}

Alternative Solutions

The most compelling use-case for deprecating types is when they are union members or interface implementations. A potential alternative would be to instead deprecate the membership/implementation instead of the type itself. The main challenge with this approach is with syntax, since it unclear how one would unambiguously annotate an interface implementation using the current @deprecated directive. Some possible alternatives:

New directive location - @deprecated on UNION_MEMBER

union Animal = Dog | Cat | Baiji @deprecated

New directive @deprecatedMembers on UNION

union Animal @deprecatedMembers(members: ["Baiji"]) = Dog | Cat | Baiji

New directive @deprecatedImplementations on OBJECT

type Baiji implements Node & Animal @deprecatedImplementations(implementations: ["Animal"]) {
  id: ID!
  name: String
}

New directive @deprecatedImplementations on INTERFACE

interface Animal @deprecatedImplementations(implementations: ["Baiji"]) {
  id: ID!
  name: String
}

Use case: objects that are members of interfaces or unions. Marking as deprecated indicates to clients that this type will no longer be returned from the server.
This can indicate to client build tooling that references to this object should be removed from client queries.
@netlify
Copy link

netlify bot commented Oct 20, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 34e5723
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/6352ed2b23b6e10008f5823b
😎 Deploy Preview https://deploy-preview-997--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@xuorig
Copy link
Member

xuorig commented Oct 21, 2022

I'd love a mechanism like this to make GraphQL continuous evolution more powerful 👍 Although it doesn't necessarily require a change to the spec, I also like that this can be used server-side, to indicate new functionality should not be added to a type that is going away.

Initial thoughts: Deprecating a type implies both deprecation all fields of that type and all spreads on that type. Should adding @deprecated on a type be invalid if @deprecated wasn't added on all fields for example? Should it be automatically inferred by clients? Automatically added by servers?

Are there weird edge cases we need to think about (All union members are deprecated, does that make the field deprecated?) Do we leave that decision to schema designers or to the client handling of deprecations or does something need to be added to the spec, etc.

@jturkel
Copy link
Contributor

jturkel commented Oct 21, 2022

Would it make sense to support deprecating the union membership or the interface implementation instead of the type? I'm thinking about use cases where a type will still be used elsewhere in the schema so it shouldn't be deprecated but you no longer want that type to be part of a given union or implement a given interface.

@xuorig
Copy link
Member

xuorig commented Oct 21, 2022

@jturkel that's a good point, we've also seen the need for union member deprecation and you're right that it doesn't always imply the full deprecation of that type. If we had that then type deprecation can be achieved through combination of field deprecations + membership deprecations? Clients directly relying on types through introspection (not through a query) for things like code generation could still gain from deprecation of the type itself I guess 🤔

@xuorig
Copy link
Member

xuorig commented Oct 21, 2022

I think what @jturkel mentioned might be easier to get into the spec, at least at first, and most likely responds to the use case more directly. Maybe we can approach it with something like this.

Unions

Requires new directive location:

union Animal = Dog @deprecated | Cat | Bird

Without new directive location

union Animal @deprecatedMembers(members: ["Dog"]) = Dog | Cat | Bird

Interfaces

type Dog implements Node & Animal @deprecatedImplementations(implementations: ["Node"]) {
  id: ID!
  name: String
}

Here we'd only be concerned with the interface implementation, and the id field would need to be deprecated separately if needed.

@jturkel
Copy link
Contributor

jturkel commented Oct 21, 2022

I guess the location of directives on type definitions makes it impossible to unambiguously annotate the interface implementation using the current @deprecated directive (unless it used prefix notation which would be confusingly inconsistent with other directives) i.e.:

# Deprecates the type rather than the interface implementation
type Dog implements Animal & Node @deprecated {
  id: ID!
  name: String
}

Will this change require introducing new introspection types and fields to model union membership and interface implementation since __Type.interfaces and __Type.possibleTypes directly return the underlying types?

@fotoetienne
Copy link
Contributor Author

fotoetienne commented Oct 21, 2022

@xuorig @jturkel 💯 deprecation of union membership/interface implementation is primarily what we're interested in. I proposed type deprecation mainly because I couldn't think of a simple way to express interface implementation deprecation in SDL. Added your syntax suggestions under "Alternative Solutions"

@xuorig
Copy link
Member

xuorig commented Oct 21, 2022

Will this change require introducing new introspection types and fields to model union membership and interface implementation since __Type.interfaces and __Type.possibleTypes directly return the underlying types?

Maybe we could add includeDeprecatedImplementations and includeDeprecatedMembers to those introspection fields similar to the way we do it for fields at the moment.

@jturkel
Copy link
Contributor

jturkel commented Oct 21, 2022

Maybe we could add includeDeprecatedImplementations and includeDeprecatedMembers to those introspection fields similar to the way we do it for fields at the moment.

That would work for including/excluding deprecated implementations/members but tooling would also want to know if a given implementation/members was deprecated or not for query linting, documentation generation, etc.

Also run prettier to fix formatting
@@ -151,6 +151,8 @@ type __Type {
ofType: __Type
# may be non-null for custom SCALAR, otherwise null.
specifiedByURL: String
isDeprecated: Boolean!
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. __Type includes non-objects (unions, interfaces, anything in __TypeKind). Maybe this field should be nullable like possibleTypes? Or defaults to false for any type that can't be deprecated?

@xuorig
Copy link
Member

xuorig commented Oct 21, 2022

That would work for including/excluding deprecated implementations/members but tooling would also want to know if a given implementation/members was deprecated or not for query linting, documentation generation, etc.

Ah yes I see what you mean now. For backward compatibility we could start by including it alongside, but yeah 😐.

@leebyron
Copy link
Collaborator

leebyron commented Nov 3, 2022

This would require adding a validation rule that a deprecated object can't be used from a non-deprecated field?

Main reason is that introspecting a schema without deprecated values should yield a valid schema.

@leebyron
Copy link
Collaborator

leebyron commented Nov 3, 2022

Note towards clarifying motivation: Use of fragment spreads of a type is why removing this directly without a deprecation direction is a breaking change and motivates having this tool

@gwardwell
Copy link

gwardwell commented Nov 22, 2022

I like the direction this proposal is taking. I would advocate for expanding the locations of @deprecated to include objects as well as handling deprecation of a specific use of an object or interface. Both are credible use cases.

In the case of deprecating an object, you could imply that an object is deprecated by deprecating all the fields of the object. But for specifications built on top of GraphQL (for example, Apollo Federation or schema stitching) inferring that an object is deprecated breaks down once the schema is extended across several services. This creates a need for deprecating the object itself so all uses of the object, both across services/schema SDL and in client applications are caught.

I agree that specific uses of objects should also be able to be deprecated. The examples of deprecating a specific interface from a specific type or a specific type from a specific union should be possible.

TL;DR I would advocate for both solutions (expanding @deprecated locations and adding new directives). I would like to see @deprecated expand to pretty much everything.

fotoetienne added a commit to fotoetienne/graphql-js that referenced this pull request Jan 6, 2023
@fotoetienne
Copy link
Contributor Author

I created a PR on graphql-js implementing the proposed solution.

@gwardwell
Copy link

This should expand to also include:

  • Deprecating scalars
  • Deprecating Input objects
  • Deprecating enums
scalar DateTime @deprecated

input BookInput @deprecated {
  title: String!
}

enum Genre @deprecated {
  SCIENCE_FICTION
}

While I realize there aren't any client benefits to deprecating these schema types, there is a great deal of service-level benefit as it will allow communication of what should or shouldn't be used in a large-scale GraphQL implementation. IDE tools could be updated to provide all the expected warnings when trying to use a deprecated scalar, for example.

@gwardwell
Copy link

Piling on here, union members and interface implementations should be able to be deprecated. Something like:

directive @deprecatedMember(member: String!, reason: String) repeatable on UNION

directive @deprecatedImplementation(interface: String!, reason: String) repeatable on Object

union PrintedMedia @deprecatedMember(member: "EReader", reason: "EReaders aren't printed.") = Book | Magazine | EReader

interface FunStuff {
  name: String!
}

type Dentist implements FunStuff @deprecatedImplementation(interface: "FunStuff", reason: "Going to the dentist isn't fun") {
  name: String!
}

For clients, such deprecation would tell them when they are querying against on of these deprecated named fragments. For example, when querying unions:

query GetPrintedMedia {
  printedMedia {
    ... on EReader {
      # This is still a valid named fragment, but client tooling could warn that
      # this member of the union is deprecated and will go away. This would allow
      # clients to move away from the deprecated union.
      # ...
    }
  }
}

Querying interfaces would behave the same way.

@martinbonnin
Copy link
Contributor

martinbonnin commented Oct 18, 2024

@gwardwell we could also explore something like:

enum __DirectiveLocation {
   # ...
   UNION_MEMBER
   IMPLEMENTED_INTERFACE
}

directive @deprecated(
  reason: String = "No longer supported"
) on 
 # ...
 UNION_MEMBER | IMPLEMENTED_INTERFACE

union PrintedMedia  = Book | Magazine | @deprecated(reason: "EReaders aren't printed") EReader 
type Dentist implements @deprecated(reason: "Going to the dentist isn't fun") FunStuff

Could that work?

Also I'm curious if other languages allow this? I get the usage but haven't seen this pattern anywhere else.

@gwardwell
Copy link

gwardwell commented Oct 18, 2024

@martinbonnin That's an interesting idea. I was trying to avoid adding new directive locations, but I'm all for it if it makes sense. I think they would need to be UNION_MEMBER and INTERFACE_IMPLEMENTATION since there's already an INTERFACE location (unless I misunderstand what qualifies as the INTERFACE location).

Regarding other languages and the deprecation of union members or interface implementations, I'm not aware of any that do it. But I'm also not aware of any that expose the union and interfaces externally through an API like GraphQL does. The motivation here is to discourage the use of inline or named fragments that explicitly reference a deprecated member or implementer. I think that use case is pretty unique.

I also opened issue #1119 to discuss a more broad changes to @deprecated, but I'd be happy to consolidate over here. My concern was that this PR had run out of steam due to lack of activity.

@martinbonnin
Copy link
Contributor

INTERFACE_IMPLEMENTATION since there's already an INTERFACE location

Good callout, I edited to IMPLEMENTED_INTERFACE (not great but at least doesn't name classh with existing locations).

@benjie
Copy link
Member

benjie commented Oct 24, 2024

I'm generally supportive of adding @deprecated to more locations, but proposing additional locations for @deprecated should be done through separate issues/PRs/RFCs (ideally one PR per location to keep the discussion on-topic). Please keep this discussion to feedback specifically related to adding @deprecated to object types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants