-
Notifications
You must be signed in to change notification settings - Fork 2k
Thought: avoiding self-repitition when a schema is defined as a string #703
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
Comments
Yeah the reason I suggested copying here was that I didn't feel like it would be good for graphql-tools (a package to write a server using the schema language) to encourage people to use a non-standard schema syntax. Currently we use the parser and schema generator from graphql-js and I'd rather not build our own. |
Thanks for sharing this idea. My sense is that this is the sort of thing that is better prototyped as a separate layer on top of existing GraphQL tooling: you could make a separate tool that would transform this specialized syntax into the more verbose canonical form, which could then be consumed by any other tools that expect to work with spec-compliant GraphQL syntax. Over time, if the utility of the idea is proven, you could look at proposing actual changes to the spec itself so that the benefits could be baked into the core language, which implementations would follow. I can certainly understand why somebody might want a composition mechanism other than fragments in order to reduce duplication and verbosity (although at the same time I wonder whether normal fragments could just be used in combination with a directive — On the concrete aspects of the proposed extension here, I'd be very careful of overloading the |
After reading this discussion, I started to wonder why types are required to list all interface fields to begin with. The initial approach looks quite reasonable to me: interface Character {
id: ID!
name: String!
friends: [Character]
appearsIn: [Episode]!
}
type Human implements Character {
starships: [Starship]
totalCredits: Int
}
type Droid implements Character {
primaryFunction: String
} In fact, I find it the most DRY notation, it already has all necessary information. Though I do see cases where it can be advantageous to repeat some of the fields. For example when a concrete type narrows down the field's result type: interface Fruit {
color: String
weight: Int
}
type Banana implements Fruit
interface Animal {
name: String
likes: Fruit
}
type Monkey implements Animal {
likes: Banana
} I also found, that it nicely translates in the standard type definitions (which are defined without IDL). For example, this is how I would normally define it in scala: trait Named {
def name: Option[String]
}
case class Dog(name: Option[String], barks: Option[Boolean]) extends Named
case class Cat(name: Option[String], meows: Option[Boolean]) extends Named
val NamedType = InterfaceType("Named", fields[Unit, Named](
Field("name", OptionType(StringType), resolve = _.value.name)))
val DogType = ObjectType("Dog", interfaces[Unit, Dog](NamedType), fields[Unit, Dog](
Field("barks", OptionType(BooleanType), resolve = _.value.barks)))
val CatType = ObjectType("Cat", interfaces[Unit, Cat](NamedType), fields[Unit, Cat](
Field("meows", OptionType(BooleanType), resolve = _.value.meows))) As you can see, I can define a scala trait When it comes to mixins, then maybe it makes sense to reuse existing concepts and allow interfaces extend other interfaces? Would love to know your thoughts on this. Though I still do have some concerns about IDL schema definition, but they are a bit different. By defining schema in a programming language, like javasctipt, one can use the full power of the language to define the schema and build abstractions. graphql-relay-js uses this a lot by providing helper functions to generate connection types, nodes, edges, etc. If schema is defined purely in IDL, these means to build powerful abstractions are gone, so one need to repeat common patterns and structures over and over. |
I agree pretty strongly with this. IDL is a great tool for describing schema in the abstract, but insufficient for implementing that schema. The tools that take in IDL and return a schema can be quite helpful with reasonable defaults until you need something beyond those reasonable defaults. |
There are a handful of reasons object types must declare the fields they're required to define via an interface.
interface MyInterface {
field: MyInterface
}
type MyObject implements MyInterface {
field(arg: String): MyObject
} This point is more important in the case a type implements multiple interfaces which both define a same named field with different (but compatible) properties.
interface MyInterface {
alpha: String
# beta: String # This field was just removed
}
# Written assuming MyObject would just automatically get MyInterface's fields:
type MyObject implements MyInterface {
gamma: String
} |
Most importantly, the IDL is designed to be read, not to be written. If fields are omitted from an object definition and assumed to be included from interfaces because it's easier to write, it adds a tax to future readers who now must follow all interfaces in order to understand all the fields defined on a type. |
I ran across these same thoughts when rewriting and updating a Node.js GraphQL server. I wanted to split the schema definition from the implementation of it so that the schema could be easily shared with other implementations and used for linting purposes etc. It is getting quite challenging for me though as the schema as its defined in the Node.js code shares the definition of fields quite frequently across interfaces and object types. At its core it basically has 2 interfaces that everything revolves around. The larger of the two has a bit over 20 fields, if which 6 or so has 2+ arguments. That larger interface is then used to create around 8 different object types and interfaces, which in Node.js was as simple as: new GraphQLObjectType({
name: 'TheNewType',
fields: documentFields,
interfaces: [documentInterface]
}); But which in the schema definition needs the duplication of all 20 fields + all their arguments across all these 8 places – making it very verbose and hard to read, write and maintain. It would have been easier + closer to the Node.js implementation if I could do something like:
That is somewhat similar to eg. traits in PHP. It would separate the implementation of fields from the definition of an interface, which @leebyron rightly points out are separate things. It would also be backwards compatible with how things are today. And most importantly: It would make things more readable as it enables types to focus on the fields that set them apart rather than having those fields become lost among the possibly many many duplicated fields. In addition it could make things more maintainable and less error prone as only the fields that are intended to be unique for a specific type are specified for that specific type, making it easier to know if a difference is an unintended typo or an actual intended difference. The GraphQL server I'm working on is maybe using interfaces and types in a slightly uncommon way – but anyhow, the sharing of all of these fields across all of those types has proven to be very powerful within the GraphQL queries, with fragments being able to target things very well, so it would be great to be able to easily reuse fields to that same degree within the standard schema definition as well. |
You could handle this outside of GraphQL. If using ES6 and graphql-tools: const MixinFields = `
id: ID
name: String
`;
module.exports = `
type Person {
${MixinFields}
}
type Dog {
${MixinFields}
}
type Query {
dog: Dog
person: Person
}
`; This approach seems to satisfy what @leebyron said in #703 (comment) |
As it always harder to discuss things in theory than to experiment with actual implementations and make conclusions of such experiments I decided to create a polyfill/transpiler for the behavior I suggested above: https://github.com/Sydsvenskan/node-graphql-partials This solved the need I had and ensured that the GraphQL schema didn't have any runtime requirements on any specific programming language. It includes a simplified example of the actual schema I'm working with: The one using "partials" is: https://github.com/Sydsvenskan/node-graphql-partials/blob/master/examples/with-partials.graphql The transpiled output is: https://github.com/Sydsvenskan/node-graphql-partials/blob/master/examples/without-partials.graphql To me the source using "partials" is much more easy to read than the one where everything is inlined. So it is inlined with what eg. @leebyron has been saying here. "partials" could be the wrong name for this, but I needed to pick something for it to work. Any feedback? |
Closing this aging issue - my comments above continue to reflect my thoughts on this. Most importantly - schema definition language is designed to be read rather than written, and following partial types and interfaces to learn what fields are defined would be a step backwards for that goal in my opinion. |
I think that in some scenarios I can overcome the original issue using Apollo’s schema directives, so yeah, I’m OK with closing this. |
This issue was originally opened in apollostack/graphql-server and apollographql/graphql-tools and was copied to this repo as a potentially more relevant place, according to @stubailo.
I quite like the idea of defining schemas simply as strings – it's great that it can be a part of the git repo in such a form. This makes the schema readable by any member of the team without running a server and also enables the new features to be tracked even by those who is not great in server-side JS (
git diff
for a spec looks pretty straightforward).A problem being introduced by this approach is that some parts of the schema become a bit more verbose than needed, e.g. when some set of fields is shared between an interface and derived types:
When I wrote my first schema in a text form, I intuitively expected this to work:
As you probably guessed, this was not very successful :–) BTW
/* GraphQL */
just helped Atom'slanguage-babel
plugin nicely highlight the types.On the yesterday's GraphQL meetup in London I shortly discussed this problem of self-repetition with @martijnwalraven and he explained me why it cannot be solved with the concept of fragments. Turns out fragments are designed to be used only on the client and their main purpose is not to reduce the number of the lines of code, but to perform some cool magic.
However, I still think that there's something that may be done in the server-side app to avoid too much repetition. It does not necessary have to be a part of the GraphQL spec – just some basic ‘syntax sugar’ will do the job (our purpose is just to make the schema definition readable by everyone including product managers).
The first thing that comes to mind is some form of a ES6 spread operator (which implements a simple
string.replace()
under the hood). A quick sketch:This may work not just for the sets of fields, but also for certain collections of commonly used attributes. Mixins (or whatever this thing may be called) can be easily made composable.
Thinking further, these mixins may be made smarter than just bare-bone
string.replace()
and even work as true spread operators. In typeCustomer
name
would turn from a locale-aware filed to a normal one. But that's probably a bit too hard conceptually for a start.Although I see certain benefits in this extra layer of abstraction, I understand that it comes at a cost, which is first of all the existence of the layer of abstraction as such. Besides, with the introduction of these mixins in the schema definition file, the resolvers might need to get something similar as well.
However, at the moment I see more pros than cons in the overall idea, especially in the long run. As the GraphQL tooling improves further, server developers will probably get more focused on the schema than on the underlying layer with resolvers, so keeping things shorter and thus more human-readable can help many teams.
I'm curious to know what the community thinks of this extra ‘syntax-sugar’ idea. Feel free to criticize it as much as you want – it's pretty raw after all! :–)
The text was updated successfully, but these errors were encountered: