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

RFC: Allow interfaces to implement other interfaces #373

Merged

Conversation

mike-marcacci
Copy link
Contributor

This fixes #295 by describing the way in which an interface may implement another interface. The rules are essentially identical to the rules governing implementation of an interface by an object. I'd be more than happy to champion this change (per the CONTRIBUTING.md process) so please direct any questions my way.

This is a pretty small change, but adds substantially to the expressiveness of GraphQL's type system.

If someone in the WG can give me a nod, I'll go ahead and implement it in graphql-js.

Thanks in advance!

@mike-marcacci
Copy link
Contributor Author

To clarify a point that came up in the WG meeting, the main use case (what can't currently be accomplished) is representing generic multi-level wrappers using interfaces. For example, representing Connection as an interface, rather than naming conventions. Without this, you're forced to choose between representing Connection and Edge as interfaces, and creating <Some Interface>Connection and <Some Interface>Edge types. Obviously the latter is universally chosen, but it seems a shame to be unable to enforce Relay's rules using the graphql type system.

# generic types
interface Node {
  id: ID!
}

interface Connection {
  pageInfo: PageInfo!
  edges: [Edge]
}

interface Edge {
  cursor: String
  node: Node
}

type PageInfo {
  hasPreviousPage: Boolean
  hasNextPage: Boolean
  startCursor: String
  endCursor: String
}


# pet-specific types
interface Pet {
  id: ID!
  name: String
}

type PetEdge implements Edge {
  cursor: String
  node: Pet
}

type PetConnection implements Connection {
  pageInfo: PageInfo!
  edges: [PetEdge]
}

type Cat implements Pet & Node {
  id: ID!
  name: String
}

type Dog implements Pet & Node {
  id: ID!
  name: String
}


# furniture-specific types
interface Furniture implements Node {
  id: ID!
  packageWidth: Number!
  packageHeight: Number!
  packageDepth: Number!
}

type FurnitureEdge implements Edge {
  cursor: String
  node: Furniture
}

type FurnitureConnection implements Connection {
  pageInfo: PageInfo!
  edges: [FurnitureEdge]
}

type Chair implements Furniture & Node {
  id: ID!
  packageWidth: Number!
  packageHeight: Number!
  packageDepth: Number!
  seatHeight: Number!
}

type Table implements Furniture & Node {
  id: ID!
  packageWidth: Number!
  packageHeight: Number!
  packageDepth: Number!
  minSeats: Number!
  maxSeats: Number!
}


# query
type Query {
  pets: PetConnection
  furniture: FurnitureConnection
}

@leebyron leebyron added 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) and removed RFC labels Oct 2, 2018
@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

I know this has been sitting for a while, but I think it has what it takes to move from Proposal to Draft.

@alloy
Copy link

alloy commented Oct 18, 2018

(@leebyron Slightly confused about the label, shouldn’t that then be Draft (RFC 2)?)

@slightlytyler
Copy link

@mike-marcacci are you still championing this? I haven't been an attendee to WG meetings before but I'd like to help in anyway to move this forward. I can't think of a single change to GraphQL that would improve its use for my team and myself more than this

@mike-marcacci
Copy link
Contributor Author

Hi @slightlytyler - thanks for the note! I am (or am at least am intending to) continue pushing for this. I missed the last WG meeting because the agenda filename was misnamed with the wrong date 😑 but am certainly planning on bringing this up at the next meeting. It seems I have some new merge conflicts to fix here, which I'll make sure to address before then.

The biggest roadblock for this RFC has actually been getting the right eyes on the js implementation I made last January. It's now all merge conflicts, so my main goal for the WG meeting is getting a commitment from maintainers to seriously consider the PR when I update it.

To be honest, I believe your appeal here – and any other votes of support – will help propel this feature forward. In past meetings the group's hesitance has not been due any technical criticism, but rather a general reluctance to introduce complexity without a clear demand. I don't totally agree that this actually adds complexity, but it is a change, and so I understand giving some pushback. So if you have a real-world use case that is made much better by this proposal, describing it here would go a long way in helping me argue for incorporation of this feature into the spec!

@slightlytyler
Copy link

slightlytyler commented Dec 13, 2018

I can give a simple(ish) example which highlights a reoccurring issue I have encountered in developing GraphQL APIs for various applications. I'll give as real-world of an example as possible without over complicating it. It's actually very close to your example.

For reference, I currently work at Docker on our distribution products. We use the concept of Node, Edge, Connection like many in the GraphQL world. We also try to use required types as much as possible, opting instead to use interfaces or unions to describe variants of a base type.

interface Node {
    id: ID!
}

interface Edge {
    cursor: String!
    node: Node!
}

interface Connection {
    edges: [Edge!]!
    page: PageInfo!
}

# ...

interface Image {
    digest: String!
    size: Int!
    # ...
}

type SingleArchImage implements Image, Node {
    id: ID!
    digest: String!
    size: Int!
    layers: [Layer!]!
}

type MultiArchImage implements Image, Node {
    id: ID!
    digest: String!
    size: Int!
    references: [PlatformReference!]!
}

# ...

type ImageEdge implements Edge {
    cursor: String!
    node: Image! # this breaks cus Image isn't a Node
}

type ImageConnection implements Connection {
    edges: [ImageEdge!]!
    page: PageInfo!
}

The above schema is close to what I would consider an optimal model, only lacking the interface composition to make it work.

interface Image implements Node {
    id: ID!
    digest: String!
    size: Int!
    # ...
}

type SingleArchImage implements Image {
    id: ID!
    digest: String!
    size: Int!
    layers: [Layer!]!
}

type MultiArchImage implements Image {
    id: ID!
    digest: String!
    size: Int!
    references: [PlatformReference!]!
}

# ...

type ImageEdge implements Edge {
    cursor: String!
    node: Image! # it should work?!
}

Let me know if this makes sense or if you require an clarification. If I can help you in anyway moving this forward please let me know

@simPod
Copy link

simPod commented Jan 4, 2019

I just stumbled upon this as well. Is there anything I can do to help to move this forward?

@mike-marcacci mike-marcacci force-pushed the rfc-interfaces-implement-interfaces branch from 4fffcd5 to 4a2aadc Compare April 27, 2019 05:47
@mike-marcacci
Copy link
Contributor Author

There is finally a new WG meeting on the schedule, so I have rebased the PR.

@leebyron per your comment above can we get this moved to draft status?

@michaelstaib
Copy link
Member

Hi @mike-marcacci,

we spoke about this item at the last wg meeting and I want to get this one further.
Do you still want to champion this?

@michaelstaib
Copy link
Member

michaelstaib commented Jun 9, 2019

@mike-marcacci @IvanGoncharov

Here is the discussion we had on the wg

  • Ivan: One concern is validation rules with fragments where there are already existing issues. We should create a prototype to gather feedback for a couple of months to gather feedback, shouldn’t keep it on stage two for now
  • Lee: Getting details right is always difficult. Good to move slow and deliberately.
  • Matt: As an example with validation rules, one piece of the interface proposal would be that we should tighten up validation rules, disallowing arbitrary spreading of interfaces. This effort is a breaking change for existing builds and should be staged.

the protocol is not yet approved but can be read in this pull request:
https://github.com/graphql/graphql-wg/pull/193/files

@mike-marcacci
Copy link
Contributor Author

Hi @michaelstaib - I am definitely committed to this. I had become a bit exasperated by the lack of participation: despite 18 months trying to get some eyes on the implementation PR, no one stepped up. I am thrilled that there's renewed interest, and will definitely start pushing this again.

Updates since the last meeting in which I participated:

  • I had to do a reworking of this RFC to fix many merge conflicts, so it needs to be reviewed again.
  • The implementation PR needs to be rewritten to address merge conflicts in the dozens of test files I modified. Then it needs to receive its first review 😉.
  • Any remaining conceptual concerns RE this RFC need to be discussed at a WG meeting. (I do think this has been discussed pretty extensively, though.)
  • Any practical concerns RE this RFC need to be addressed at a WG meeting:
    • Interfaces will now appear in an introspection query in places that currently only have concrete types; how might this effect tooling?
    • Is there a conflict with arbitrary spreads here? I don't immediately see that there is anything particularly relevant in this PR, but perhaps I'm missing something. Either way, I'm generally very open to adopting stricter validation rules. One thing we should generally keep in mind while doing this, though, is not breaking the schema evolution practice in which a concrete type is changed into an interface (ie. User evolves from a concrete type to an interface implemented by Bot, Individual, and Organization concrete types).

I will add this to the next meeting's agenda and make sure I'm available to join. If there's any interest in discussing this or the proposed validation rules in an informal ad-hoc call, let me know and I'll make it happen.

🎉🎉🎉

@mike-marcacci
Copy link
Contributor Author

Copying those who participated in the last WG discussion according to the (AMAZING) WG agenda notes: @michaelstaib @leebyron @IvanGoncharov @mjmahone

Tapped added a commit to cognitedata/sangria that referenced this pull request Mar 9, 2021
Tapped added a commit to cognitedata/sangria that referenced this pull request Mar 9, 2021
@leebyron leebyron added this to the May2021 milestone Apr 6, 2021
@yaacovCR
Copy link
Contributor

yaacovCR commented Apr 4, 2022

@mike-marcacci should unions also be allowed to implement interfaces for all the same reasons and in the exact same way?

#518 (comment)

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

Successfully merging this pull request may close these issues.

Proposal: Allow interfaces to implement other interfaces