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

Mixin Support #370

Closed
tylercrompton opened this issue Oct 3, 2017 · 11 comments
Closed

Mixin Support #370

tylercrompton opened this issue Oct 3, 2017 · 11 comments

Comments

@tylercrompton
Copy link

tylercrompton commented Oct 3, 2017

Introduction

When using GraphQL, I find myself often writing duplicate code in field definition maps (i.e. the field definitions in interface definitions, Object definitions, and Input Object definitions), especially when implementing interfaces. I think that the inclusion of mixins would aid in schematic maintenance by allowing for a reduction in such duplicate code, especially in large schemata. Though a mixin can technically be defined as an interface and be included in a field definition map via a directive without any major changes to the GraphQL grammar itself, I think that a dedicated syntax would aid in readability because mixins are not conceptually identical to interfaces (among other reasons).

Consider the following example:

My company wants to store and serve various metadata on each Object. I could create an interface that describes the schema of this metadata as follows:

interface Metadata {
    createdAt: Int!
    createdBy: User!
    updatedAt: Int
    updatedBy: User
    deletedAt: Int
    deletedBy: User
}

But then, I would must verbosely implement the Metadata interface in each Object that claims to implement it, and I would be unable to implement other, more relevant interfaces in those Objects. Should I find myself wanting to change one of the field definitions, I'd have to change it in several places throughout the schema.

A workaround is to implement a new @mixin(name: "InterfaceName") directive which would be used in place of the field definitions in the implementing Objects. This would allow for the reduction of duplicate code as the following snippet demonstrates:

type Entity1 {
    id: ID!
    name: String!
    @mixin(name: "Metadata")
}

However, the aforementioned technique is inconsistent with the current GraphQL grammar, because it wouldn't convey that mixins are conceptually very similar to fragments, which already have a dedicated syntax.

Proposal

Therefore, I propose the following new syntax:

Mixin Definition

mixin Metadata {
    createdAt: Int!
    createdBy: User!
    updatedAt: Int
    updatedBy: User
    deletedAt: Int
    deletedBy: User
}

This mixin definition syntax is straightforward, self-explanatory, and very readable.

Mixin Inclusion

type Entity {
    id: ID!
    name: String!
    ...Metadata
}

This mixin spread syntax borrows the spread operator from the fragment spread syntax because it's readable, it's familiar, and it's analogous to fragment spreads. Because fragment spreads are invalid in field definition maps, and mixin spreads are invalid in selection sets, this syntax won't introduce any conflicts in the grammar.

Implications

I believe that these proposed changes to the GraphQL grammar will be relatively easy to implement, will allow more concise code, and will provide a better development experience. I'll close by demonstrating the utility of these proposed changes via the following example schema:

mixin Timestamps {
    createdAt: Int!
    updatedAt: Int
    deletedAt: Int
}

mixin Mutators {
    createdBy: User!
    updatedBy: User
    deletedBy: User
}

mixin Metadata {
    ...Timestamps
    ...Mutators
}

mixin Sluggable {
    slug: String!
}

interface Submission {
    id: ID!
    body: String!
    ...Metadata
}

type User {
    id: ID!
    username: String!
    ...Timestamps
}

type Post implements Submission {
    id: ID!
    title: String!
    body: String!
    ...Sluggable
    tags: [Tag!]
    ...Metadata
}

type Comment implements Submission {
    id: ID!
    body: String!
    post: Post!
    ...Metadata
}

type Tag {
    id: ID!
    title: String!
    description: String
    ...Sluggable
    ...Metadata
}
@swyxio
Copy link

swyxio commented Oct 12, 2017

been 9 days. what are the community norms for discussing these proposals?

@SerafimArts
Copy link

@sw-yx I wrote a proposal for the implementation of LSP and the hierarchy of types a month ago, for this month no one even asked "wtf, what it is!11??". It seems to me that GraphQL as a standard is more likely to be dead than alive.

@SerafimArts
Copy link

SerafimArts commented Oct 27, 2017

In any case, it's easier to:

  1. Make the implementation of the interface simply not necessary
  2. And the inheritance of interfaces
interface Timestamps { ... }
interface Mutators { ... }

interface Metadata extends Timestamps, Mutators {}

# ...
type Comment implements Metadata {
    id: ID!
    body: String!
    post: Post!
}

Thus, we get the same result, but at the same time we use existing constructions (interface instead mixin)

@voxpelli
Copy link

voxpelli commented Nov 1, 2017

I've also desired this and in practice pretty much needed it – so I made myself a transpiler/polyfill for it, using a slightlty different syntax: https://github.com/sydsvenskan/node-graphql-partials (I took more inspiration from how eg. traits works in PHP than from how eg. mixins work in Sass as the proposal here)

I posted a bit more about my transpiler/polyfill in this thread, which also discusses this kind of behavior: graphql/graphql-js#703 (comment)

This is also slightly related to #295, where this has been brought up before

@tylercrompton
Copy link
Author

@SerafimArts, I like the idea but my only aversion to that is that I don't think implements would quite be the right keyword for such behavior. Your proposal is an argument for abstract types in addition to inheritance, so though it aims to solve a similar problem, it's conceptually quite different, so if this is something that you'd like to discuss, I think that a new ticket would be fitting. 😊

@SerafimArts
Copy link

@tylercrompton It seems to me that this is just a solution to the same problem, but a little more complex/global, at the level of the entire architecture of the SDL language. In the case of "mixins", this is another additional construction that serves only one - simply supplement the body with other inserts.

@leebyron
Copy link
Collaborator

leebyron commented Dec 1, 2017

I'm going to close this issue for repo maintenance but here are some of my opinions on this topic and suggestions for how to move forward:

First just a point of clarification for the schema definition language. The expectation is that SDL will be read far more often than it is written (in fact in many cases it is even "written" by a code generation script) and therefore is optimized for legibility over terseness. It is valuable to see all fields defined on a type without having to follow references to mixins or interfaces. However for cases where you are writing more often than reading (perhaps for quick prototyping or using tools like AppSync or Graphcool) then I certainly see the appeal for having tools to allow for more terse representations.

Spec proposals typically intentionally lag behind real world experimentation. The goal for the GraphQL spec is to dictate what is required to be considered GraphQL by codifying the most successful explorations into new capabilities by GraphQL services and tools. That means new ideas are best served by being built out and used for their intended purpose and learned from before being codified in spec.

If adding mixin support is important to you, I would recommend you build it! Try it out and learn along with the community and if that lends itself to broad support then a spec proposal would be a good final step to codify the best version.

@leebyron leebyron closed this as completed Dec 1, 2017
@tylercrompton
Copy link
Author

The expectation is that SDL will be read far more often than it is written

That is a very fair statement. However, can the same not be said about GraphQL operations (i.e. queries, mutations, and subscriptions)? Those of course can have fragments which are synonymous to mixins. What differentiates fragments from mixins in regard to readability and maintainability? Operations themselves are typically written more often than types and interfaces, but I would bet that given any one type or interface and any one operation of comparable complexity, one would be read or changed just as frequently or rarely as the other on average.

If adding mixin support is important to you, I would recommend you build it!

I am by no means against doing this and would be glad to report my findings. I actually created a ticket for it in my project's issue tracker immediately after opening this issue. However, beyond writing a formal specification, I'm not sure how to best proceed, considering that my implementation would be dependent on the library on which it builds. Thus, the potential audience who could try it out in their projects would be limited. I'd hate to build it and be the only one to report their findings. I'm obviously already biased to supporting them. Do you have any suggestions as to how to proceed in this regard?

@voxpelli
Copy link

voxpelli commented Dec 5, 2017

If adding mixin support is important to you, I would recommend you build it!

I've already done it: https://github.com/sydsvenskan/node-graphql-partials As I mentioned in #370 (comment)

@tylercrompton Please try my piece of code out and we can maybe get some common findings

@yaquawa
Copy link

yaquawa commented Oct 13, 2018

@leebyron

The expectation is that SDL will be read far more often than it is written

Well, that's why we should have the mixin…
It also increase the readability which fits the expectation.

in fact in many cases it is even "written" by a code generation script

Yeah, but with the mixin, you can make the the parsing performance better, and also decrease the introspection response size.

Prisma generates ton of duplicated schema like PostCreateWithoutAuthorInput is just PostCreateInput with out the author_id, but you are forced to write all the fields of Post again. If there is one or two object, it's fine, but how about 10? 20? 100? every relation has a XXXCreateWithoutXXX input type, and you can easily imagine how the combination will go explosion.
Even you get the program like Prisma generate the schema, but the parsing cost will increase very fast, so does the introspection response size.

There should have some OOP concepts like A extends B or mixin to increase the readability, and more importantly, decrease the parsing cost and introspection response size.

I think it's far more readable than just make the object like a god object with all the fields it has.
But if you really want to all the fields in the object, you can just use any parser to generate that kind of writing style. rather, you can just use IDEs to jump into a mixin or parent to exploring the object fields.

I think having the mixin and inheritance have more benefits than just keep all the fields in the object, after all, having the mixin and inheritance doesn't mean you are forced to use them, if you like to keep all the fields in the object completely, you can do that as well.

@SerafimArts
Copy link

SerafimArts commented Oct 13, 2018

Hmm, but what about this case?

# The ability to use any type instead of keyword "mixin"
type AbstractMessage {
    id: ID!
    body: String!
    user: User!
    createdAt: DateTime!
}

type UserMessage {
    ...AbstractMessage {
        # Need the ability to specify where exactly the field will be located
        createdAt @after(field: "chat")
    }

    chat: ChatThread!
}

type MailMessage {
    ...AbstractMessage {
        createdAt @after(field: "user")
        # Need the ability to exclude unnecessary fields
        user @skip(if: true)
    }

    email: String!
}

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

No branches or pull requests

6 participants