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: Add Interface inheritance for definitions #335

Closed
SerafimArts opened this issue Aug 3, 2017 · 9 comments
Closed

Proposal: Add Interface inheritance for definitions #335

SerafimArts opened this issue Aug 3, 2017 · 9 comments
Labels
🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)

Comments

@SerafimArts
Copy link

Spec (http://facebook.github.io/graphql/#sec-Interfaces) does not proposes this feature. But this is will be good for ISP (Interface Segregation Principle) as example.

Example:

Base definitions

interface Creatable {
    createdAt: String!
}

interface Updatable {
    updatedAt: String
}

Variation of implementation

interface Person extends Creatable, Updatable {
    id: ID!
    name: String!
    # ...
}

class User implements Person {
    # Impl of interface fields: 'id', 'name', 'createdAt', 'updatedAt'
}

Interface fields overriding

Discussion needed. I think. e.g. how to consider the LSP (Liskov Substitution Principle)?

  1. Allows NonNull for Nullable fields?
  2. Allows ListType for "Single" type def?
  3. Allow to increase/decrease the number of arguments for field input?
# Example
interface HasDates extends Creatable, Updatable {
    updatedAt: String! # Overriding nullable field
}

Errors

  1. Error while cyclic inheritance
interface A extends B { ... }
interface B extends A { ... }
  1. Error inheritance twice
interface Person extends Creatable, Updatable { ... }

type User implements Person, Creatable {
    # Error: Can not implement "Creatable" interface. Interface already defined in Person.
}

Like everything foreseen, there are ideas? =)

@SerafimArts
Copy link
Author

Ooops... It seems like a duplication of #295

@tylercrompton
Copy link

tylercrompton commented Oct 3, 2017

It's similar but not related. Inheritance implies that the parent interface's fields are implicitly included in the child interface's schema. Additionally, you've touched on the topic of multiple inheritance and overriding properties of inherited fields. Neither of these are proposed in #295. I think that it's fair to say that this is different enough to not be a duplicate. However, the proposals conflict (in that they accomplish similar goals in different ways) and thus should be considered as mutually exclusive.

@SerafimArts
Copy link
Author

SerafimArts commented Oct 3, 2017

@tylercrompton Convincingly, I'll reopen ann issue =)

However, I note that interfaces just exist to solve the problem of multiple inheritance and attach this link here: https://en.wikipedia.org/wiki/Interface_segregation_principle

At the moment the interfaces are a rudiment without the corresponding functionality.

@SerafimArts SerafimArts reopened this Oct 3, 2017
@tylercrompton
Copy link

tylercrompton commented Oct 4, 2017

I think that for the sake of development and transition, we should consider deferring multiple inheritance and possibly "strictification" of types as well to a separate issue, pending the adoption of the simplified version of your proposal. In other words, I think that this proposal should focus solely on regular inheritance (for now) for the sake of getting at least something workable approved.

Additionally, your proposal introduces the diamond inheritance problem since types can implement multiple interfaces. We need to address this by identifying the desired mitigation algorithm.

@Recio
Copy link

Recio commented Jun 19, 2018

So according to this issue, it seems that implicit inheritance is still not supported?

Is it strictly necessary to declare all the fields when implementing an interface?
That seems to lead to a lot of duplicated code...

There is no extension / polymorphic mechanism with implicit field inheritance in graph?

@SerafimArts
Copy link
Author

SerafimArts commented Jun 19, 2018

@Recio You can use custom directives to implement this behavior. This is exactly what I did:

directive @extends(type: String!) on OBJECT

type Some { ... }
type Some2 @extends(type: "Some") { ... }

But I had to get inside the building pipeline to implement the logic of these directives.

@Recio
Copy link

Recio commented Jun 19, 2018

@SerafimArts I'm a bit of a newb on the matter, but I suppose on build you inject the code from Some into Some2 ?

@SerafimArts
Copy link
Author

@Recio in my case everything is different. I do not know how it works in the JS, because I implement my own SDL compiler =)

@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

See: #500 (comment)

TL;DR this proposal doesn't allow new use cases that were not already possible and degrades readability of schema in the name of writing less, which goes against GraphQL's guiding principles

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

No branches or pull requests

4 participants