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: "and" (or product) types. #247

Closed
AndrewIngram opened this issue Dec 1, 2016 · 8 comments
Closed

Proposal: "and" (or product) types. #247

AndrewIngram opened this issue Dec 1, 2016 · 8 comments
Labels
🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)

Comments

@AndrewIngram
Copy link

AndrewIngram commented Dec 1, 2016

When building large schemas using the schema language patterns, it's relatively awkward to build the mutation type. My definitions are spread across a large number of files, and I typically want my mutations to be defined alongside the types they are most related to.

We're restricted to having a single Mutation type, with no namespacing, that has to hold all the mutations for the entire schema, this makes it relatively awkward to define mutations in a modular way without resorting to some trickery.

I'm proposing something like this:

type User {
  id: ID!
  username: String
}

type UserMutation {
  createUser(username: String!): User
  updateUser(id: ID!, username: String!): User
}

type Article {
    id: ID!
    author: User!
    title: String!
    body: String!
}

type ArticleMutation {
  createArticle(authorId: ID!, title: String!, body: String!): Article
  updateUser(id: ID!, authorId: ID, title: String, body: String): Article
}

type Mutation = UserMutation & ArticleMutation

Aside from the syntactic sugar provided by the last line, this is something I already do as a post-processing step after generating my schema. But i'd prefer to do away with the hack entirely and have a solution that feels more standardised.

The basic spec would be that an and type combines all the fields of the types being added, and errors on a collision.

@stubailo
Copy link
Contributor

stubailo commented Dec 1, 2016

What about an alternative - open types?

So:

type Mutation {
  createArticle(authorId: ID!, title: String!, body: String!): Article
}

type Mutation {
  createUser(username: String!): User
}

Could just merge into one type by default.

That seems to address the current issue more directly.

@AndrewIngram
Copy link
Author

Hm, I worry about the lack of explicitness with that approach. Is it actually my intent to merge the two types? Or have I accidentally created a 2nd type with a name that clashes with another?

@stubailo
Copy link
Contributor

stubailo commented Dec 1, 2016

That's true, it would be possible to make a mistake of that nature with that approach. I'm a bit worried that the type merging approach will result in tons of unused types in the schema though, like if you generate docs they will have many different mutation types.

Two other options:

# Explicitly declare that we are reopening a type
extend type Mutation {
  createArticle(authorId: ID!, title: String!, body: String!): Article
}
# Type fragments
type fragment UserMutation {
  createUser(username: String!): User
}

type fragment ArticleMutation {
  createArticle(authorId: ID!, title: String!, body: String!): Article
}

type Mutation {
   ...UserMutation
   ...ArticleMutation
}

The fragment thing is essentially the same as your proposal, except that the fragments wouldn't be listed in the documentation as their own types. Or perhaps there is some other way to mark types as "unlisted".

I feel like having a way to declare snippets of fields would be pretty valuable, for example ...Timestamps or something.

@rmosolgo
Copy link

rmosolgo commented Dec 6, 2016

✋ as a Ruby developer ... open classes seems like a lot of fun until it's not! 😆 Just for all the same reasons described above: strange coupling between parts without any indication in the source code itself.

I prefer the type fragment behavior. In fact, graphql-ruby does this by default with Interfaces: the interface's field implementations are "inherited" by the object types. I'm not sure it's a feature, because it makes it possible to accidentally fulfill an interface. But I was looking for something like the ...Timestamps you mentioned!

@stubailo
Copy link
Contributor

stubailo commented Dec 6, 2016

Yeah the only unfortunate part with the type fragments thing is that it (I think) requires changing the parser, so we can't easily polyfill it.

@stubailo
Copy link
Contributor

stubailo commented Dec 6, 2016

Confirmed: https://astexplorer.net/#/HHyDzIQq7H

@jamesgorman2
Copy link

jamesgorman2 commented Dec 10, 2016

I've been experimenting with the idea of merging schema fragments over in https://github.com/jamesgorman2/graphql-type-repository . I've only got a (non-runnable since I've not finished it) code example up, not a full readme. The intuition is that type that becomes query/mutation/subscription isn't really a since all of the root accessors get dumped in there (or if you prefer, it's the union of every aggregate type in your graph). This means we can freely merge all the fields for each of query/mutation/subscription and emit a schema with root type name that is independent of any of the individual contributors, Most of the work is then around making sure your graph is complete and internally consistent, and reporting exactly what went wrong if it isn't.

An extension to this was that since the internals are based around decorating immutable type graphs until they are complete (see Internal repository execution in the readme), you could add a decorator that namespaces each of your query/mutation/subscription fields with (say) the module name or the output type name for better API clarity.

@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

This seems similar to #48, but seems to be only syntax sugar instead of allowing new use cases.

A reminder that any decision that compromises between readability and writability, I think readability should always be the priority.

Since this proposal is syntax sugar that makes reading the schema language more difficult, I think it's appropriate to move this to rejected.

@leebyron leebyron added the 🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md) label Oct 2, 2018
@leebyron leebyron closed this as completed Oct 2, 2018
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

5 participants