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

Proposal: Allow interfaces to implement other interfaces #295

Closed
mike-marcacci opened this issue Apr 3, 2017 · 20 comments · Fixed by #373
Closed

Proposal: Allow interfaces to implement other interfaces #295

mike-marcacci opened this issue Apr 3, 2017 · 20 comments · Fixed by #373

Comments

@mike-marcacci
Copy link
Contributor

mike-marcacci commented Apr 3, 2017

This proposal expands upon the ideas described in #294 (which ended up already being implemented).

I'd like to suggest that GraphQL be expanded to allow an interface to implement another interface. This would allow for higher order interfaces and open the door for frameworks and tooling to describe their requirements in GraphQL, which would be implemented in userland interfaces and types.

Here's a (very contrived) example borrowed from #294 and expanded upon to show how hierarchies would exist:

interface Pet {
	id: !ID
	sex: Sex
	name: String
	mother: Pet
	father: Pet
}

interface Equine implements Pet {
	id: !ID
	sex: Sex
	name: String
	mother: Pet
	father: Pet
	hands: Float
}

type Horse implements Equine {
	id: !ID
	sex: Sex
	name: String
	mother: Horse
	father: Horse
	hands: Float
}

type Donkey implements Equine {
	id: !ID
	sex: Sex
	name: String
	mother: Donkey
	father: Donkey
	hands: Float
}

type Mule implements Equine {
	id: !ID
	sex: Sex
	name: String
	mother: Equine
	father: Equine
	hands: Float
}

What's important here is that in a context where we generically have Pets, each pet's father is only guaranteed to be a Pet. When we know we have a Mule, though, we know that its father is an Equine.

With these two features, tools which currently depend on schema conventions (like the Relay specs) could codify their requirements with GraphQL.

A Real-World Case

I have been working on a system which allows offline, concurrent modifications to versioned documents. The problem and solution are very generic, but the current state of the GraphQL type system doesn't allow me to describe these general-purpose shapes. Much like the Relay team had to define a convention for any type ending in Connection, I have an internal spec describing any type with a name ending in Object, Entity, and Change. So, for example, I have a HikeObject, HikeEntity, and HikeChange; I also have a ClimbObject, ClimbEntity, and ClimbChange.

I've found myself wishing I could use the GraphQL type system to describe these requirements, and because the current spec is so close, I find myself thinking of the schema in these terms.

This would make that possible, and bring GraphQL's type safety to tools that solve generic problems with GraphQL.

@mike-marcacci
Copy link
Contributor Author

As I've continued the think about this, I wanted to clarify a point:

If an interface implements another interface, it MUST specify compatible fields in its own fields list.

This keeps all the simplicity of the current system (with the total absence of inheritance, and a flat interface implementation strategy) while adding the functionality of chained interfaces.

@nicksrandall
Copy link

I also have a "real world" use case for this and would love to see this added to the spec. What are the concerns?

@stanishev
Copy link

stanishev commented Sep 15, 2017

@mike-marcacci, only a suggestion: would it be possible to consider using extends instead of implements?
I am relatively new to graphql, but I find that most devs coming from any language or environment think of interfaces extending other interfaces, while types and classes implement interfaces. I think this would apply even in the scenario above where you mention that interfaces down the chain MUST specify all fields in their own fields list.

@mike-marcacci
Copy link
Contributor Author

Hi @stanishev! Thanks for the comment – I definitely thought about using this terminology, and considered how it might be interpreted by programmers of various languages. When writing this issue, I decided against extends to avoid the misconception that I was advocating for inheritance.

In fact, it's quite possible that there is a use-case for true inheritance (by which I mean applying the field definitions of one or more parents), while my use case simply needs the ability to describe a hierarchy. I would argue that extend is a different potentially-useful feature, in which a concrete type might define its fields by reference to other concrete types. I wanted to avoid misappropriating this word to describe a concept that's already fully contained by the implements keyword in GraphQL (albeit currently only for concrete types).

Also, I wanted to avoid bringing into the discussion the complexities of inference in general: In what order of precedence might a multi-parent inheritance effected? What happens when parents have non-identical (but potentially compatible) field types (type variance)? Can I "sub-class" a parent to have a non-compatible field type? etc...

All that said, if the GraphQL team chose to add this to the spec under the keyword extends, I would be more than happy to have it. I do think, though, that reusing implements here is a bit smaller of a request :)

@vergenzt
Copy link

vergenzt commented Oct 2, 2017

Not sure how mixins/inheritance got involved, but could that discussion happen in a new thread if you'd like to advocate for it? Either of those concepts would be more complicated than what this issue proposes, and I'm very interesting in seeing this proposal added so would rather not see the discussion veer off.

(Mixins sound interesting, but the GraphQL team seems acutely interested in keeping the spec as simple as it can possibly be, and IMHO mixins are not simple.)


Any thoughts from the core team on this? Would you be open to a PR? I'd love to help @mike-marcacci out with this but wouldn't want to waste the effort if you think it's unlikely to be accepted.

@MatthewHerbst
Copy link

While this is being discussed, does anyone have a clean workaround that they are using?

@jwb
Copy link

jwb commented Oct 29, 2017

I would also make use of this at Zillow, where our type system includes a hierarchy of property records, in which the most basic type is a PublicRecord, which is extended by a PropertyRecord (includes attributes added by Zillow systems), which in turn is extended by a PropertyListing (includes attributes added by an agent or owner).

Is anyone already working on a PR to incorporate this proposal into the spec?

@voxpelli
Copy link

voxpelli commented Nov 1, 2017

@MatthewHerbst @jwb While I don't want to reintroduce mixins/partials as a solution to this issue, I do wanted to point you to graphql/graphql-js#703 (comment) and the solution I outlined there and the transpiler/polyfill I created to solve my need for making my schema more DRY by adding mixins/partials: https://github.com/Sydsvenskan/node-graphql-partials

(It's a solution that would also help in making heavy usage of interfaces manageable – ensuring all implementations are in sync with the fields defined)

@tylercrompton Did you create another issue for the mixin case? Would be good to link graphql/graphql-js#703 to that one if so

@voxpelli voxpelli mentioned this issue Nov 1, 2017
@jwb
Copy link

jwb commented Nov 1, 2017

If mixin support were added, I would make use of it. I feel that enabling inheritance of interfaces is a basic feature of a type system and I need it more than I need mixins, since I can accomplish what I would use from mixins (minus inheritance) with code generation.

@mike-marcacci
Copy link
Contributor Author

For anybody following along, I went ahead and made a PR to add this to the spec: #373

If I can get some kind of indication from the GraphQL WG that this would be considered, I'll go ahead and implement it in graphql-js.

@leebyron
Copy link
Collaborator

Thanks for pressing forward with this, @mike-marcacci, and my apologies for not addressing this PR much earlier. Perhaps you could speak to this at the next GraphQL working group meeting (I'm working on putting up an agenda for that today).

I believe we considered this during the original design and turned it down for YAGNI reasons. It's just much simpler to have Object types describe all the interfaces they implement directly rather than having to walk a hierarchy. I think having concrete examples of type systems which are challenging or unsustainable without this feature could make a really compelling argument.

My opinion based on what you've drafted up so far is that it's eminently reasonable, and requiring implements and explicitly defining the fields solves any multiple inheritance issues. I think the true cost of this is only partly complexity cost, but mostly change cost. Tools built around interfaces would need to expand to support this, and that's non-trivial. So we should just ensure the value it provides would be worth that cost.

@mike-marcacci
Copy link
Contributor Author

@leebyron thanks so much for the very thoughtful response! I'll definitely join the next WG meeting if that would be helpful (I just subscribed to the wg repo so I can add myself once the agenda is up).


It's just much simpler to have Object types describe all the interfaces they implement directly rather than having to walk a hierarchy.

I thought about requiring implementing types to re-declare indirectly-implemented interfaces. In the toy example above, then, Horse, Donkey, and Mule would all need to specify: implements Equine, Pet. I decided against this, though, since there is an ergonomic cost when writing/maintaining a schema, and wouldn't make improvements on the change cost.


So we should just ensure the value it provides would be worth that cost.

I definitely understand the resulting change cost here. I skimmed through the WG notes and CONTRIBUTING.md but didn't see any formal policy on how/when breaking changes are applied to the spec.

I'll work on a companion PR to graphql-js and dive into relay to get a better understanding of what kind of changes this would require.

mike-marcacci added a commit to mike-marcacci/graphql-wg that referenced this issue Jan 28, 2018
Adding myself [as suggested](graphql/graphql-spec#295 (comment)) to talk about allowing interfaces to implement other interfaces.
@mike-marcacci
Copy link
Contributor Author

This is implemented in graphql/graphql-js#1218

@mike-marcacci
Copy link
Contributor Author

mike-marcacci commented Feb 3, 2018

Per discussion at this week's WG meeting, I set out to better describe my real-world case here as a justification for this feature, as opposed to the highly contrived example from this issue.

Here's the essence of my use-case, but using Connection terminology to avoid having to explain the intricacies of my system:

interface Node {
  id: ID!
}

interface Edge {
  cursor: String
  node: Node
}

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

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

interface NamedNode {
  id: ID!
  name: String
}

type NamedEdge implements Edge {
  cursor: String
  node: NamedNode
}

type NamedConnection implements Connection {
  pageInfo: PageInfo!
  edges: [NamedEdge]
}

type SomeNamedThing implements NamedNode & Node {
  id: ID!
  name: String
}

type Query {
  someQuery: NamedConnection
}

Note that this schema fails validation with:

[
  {
    "locations": [
      {
        "column": 9
        "line": 7
      }
      {
        "column": 9
        "line": 29
      }
    ]
    "message": "Interface field Edge.node expects type Node but NamedEdge.node is type NamedNode."
    "path": [undefined]
  }
]

the solution is simply to allow NamedNode to declare that it implements Node:

interface NamedNode implements Node {
  id: ID!
  name: String
}

Using this updated schema in the proposal's branch of graphql-js works exactly as expected and is able to simultaneously communicate that:

  1. NamedConnection is intended to be used as a Connection by the generic pagination code
  2. Any NamedConnection will only have NamedNode results, and can be queried without having to use fragment type conditions:
query {
  someQuery {
    edges {
      node {
        name
      }
    }
  }
}

Now for the twist.

After the WG meeting, I set out to show how my problem could not be solved with the current spec; but instead, I found another way to accomplish my goals. Armed with a much better understanding of introspection (thanks, ironically, to implementing this feature in graphql-js) I realized that it is quite trivial to determine all the possible types of a field, and inject the appropriate type-conditional fragments (which was essentially my use case). Given the allegory above, I essentially decided that I didn't really need the Connection or Edge interfaces at all.

The one remaining itch for me is that there is currently no way to indicate within the schema that a type is intended to be used in a generic capacity apart from naming conventions. This however, could be solved by exposing type-level metadata, which is a feature that appears to enjoy wide support from the WG.

EDIT: I've circled back to this proposal. While queries can be generated without this, you lose out on the expressiveness and type guarantees that graphql provides natively. Hierarchal interfaces would cause compile-time validation errors for invalid schemas, which are far preferable to runtime bugs.

So while I do believe this proposal generally improves the expressiveness of GraphQL and I think the example above clearly demonstrates a limitation of the current spec, I am going to ease off my campaign for it.

I'd like to leave these issues/PRs open until the next WG meeting to allow anybody else with a real-world use case to chime in. I'd also really be interested in any estimations of change cost, particularly from the perspective of tooling maintainers.

If we don't have a compelling case for this change at the next meeting, I'll happily close these issues :-)

@MatthewHerbst
Copy link

@mike-marcacci Great post. Can you please show any of the code you used to achieve the workaround?

@mike-marcacci
Copy link
Contributor Author

Hey @MatthewHerbst, I’m not actually using this for connections (I let relay handle that) but instead custom wrapper types for revision control & offline support, so none of my code is going to make sense without a lot of explaining. However, I can tell you that I essentially keep a list of Entity types (analogous to Connection implementations above) that should support offline behavior. I then traverse the introspection looking at all the possibleTypes for instances of these. Then, when proxying queries, I inject type-conditional fragments where applicable. This has several drawbacks: I might have to inject several type-specific fragments instead of 1 interface-specific fragment (which would be fixed by this issue); but I also run the risk of causing field merge issues (see #399). I’m not particularly happy with my solution anymore, to be honest, so when I have time to revisit it I’m going to look at doing things at compile-time (a la Relay).

@axos88
Copy link

axos88 commented Feb 15, 2018

Another use, also tied with pagination is when different interfaces need to be paginated.

Suppose we have an interface Profile, and two implementations: UserProfile and TeamProfile

Within the current featureset, I cannot see a solution to be able to deduct that UserProfilePagnated and TeamProfilePaginated can be supplied where we are expecting PaginedProfiles.

My first thought was generics and covariance:

With generics - although not trivial, one can make a complex analyzer that understands the notion of co and contravariance, and can deduct that PaginatedProfiles is an interface of UserPaginatedProfiles, although one might need to signal this somehow.

But @mike-marcacci's interface extensions could also be a solution, if the interfaces redefined the pagination fields

@caub
Copy link

caub commented Apr 4, 2018

Github GraphQL API uses interfaces https://developer.github.com/v4/interface

reconbot added a commit to reconbot/graphql-tools that referenced this issue Apr 6, 2018
This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema`

Later on we'll want to support interfaces implementing other interfaces
graphql/graphql-spec#295
reconbot added a commit to reconbot/graphql-tools that referenced this issue Apr 6, 2018
This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema`

Later on we'll want to support interfaces implementing other interfaces
graphql/graphql-spec#295
reconbot added a commit to reconbot/graphql-tools that referenced this issue Apr 6, 2018
This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema`

Later on we'll want to support interfaces implementing other interfaces
graphql/graphql-spec#295
reconbot added a commit to reconbot/graphql-tools that referenced this issue Apr 6, 2018
This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema`

Later on we'll want to support interfaces implementing other interfaces
graphql/graphql-spec#295
reconbot added a commit to reconbot/graphql-tools that referenced this issue Apr 10, 2018
This adds `inheritResolversFromInterfaces` to `addResolveFunctionsToSchema` and `makeExecutableSchema`

Later on we'll want to support interfaces implementing other interfaces
graphql/graphql-spec#295
benjamn pushed a commit to ardatan/graphql-tools that referenced this issue Apr 10, 2018
This adds a `inheritResolversFromInterfaces` option to `addResolveFunctionsToSchema` and `makeExecutableSchema`.

Later on we'll want to support interfaces implementing other interfaces: graphql/graphql-spec#295
@jwb
Copy link

jwb commented May 9, 2018

I'm sorry that I can't point at code, but I really want to my schema to express the relationship:

location <|- property <|- listing

@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

In an effort to reduce the simultaneous channels of discussion, I'm going to close this issue in favor of the open #373 where this RFC has continued to develop.

@leebyron leebyron closed this as completed Oct 2, 2018
mattstern31 pushed a commit to mattstern31/graphql-wg-membership-note that referenced this issue Dec 1, 2022
Adding myself [as suggested](graphql/graphql-spec#295 (comment)) to talk about allowing interfaces to implement other interfaces.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants