-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[WIP] New schema for all query/schema examples #44
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
Conversation
Notes from @tmeasday:
|
@@ -85,7 +90,7 @@ That means that the GraphQL service needs to have a `Query` type with `hero` and | |||
```graphql | |||
type Query { | |||
hero(episode: Episode): Character | |||
droid(id: ID!): Droid | |||
droid(id: String!): Droid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to encourage the best practice of using ID type for id fields/arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I noticed that the old example schema had String instead of ID, but let's upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why that is a best practice - the only benefit of ID is that you can pass it an int and it will automatically cast to a string, right? normally you would not want to do that in your schema AFAICT. if it's a string in your database it seems safer to have it be a string in graphql.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't really see the benefit of IDs, but it seems like a lot of people value them. Dan Schafer went out of his way in his React Europe talk to mention how non-user-readable IDs should be treated specially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being more generally accepting of a wider range of input is one benefit, but the primary benefit is the semantic value. It distinguishes from strings you might display to a user.
It also helps in migrations when you might start using Int based ids but later realize the scale problem and want to migrate to 64int or String based ids. Or when you need to unify multiple systems which use different storage types for their ids. This is a very real problem we faced at Facebook many times, and so making String/Int backing an implementation detail of the semantic ID offers more freedom for implementors and shields clients from that
Perhaps for mutations you can add a rating to a film and query ratings? That seems more realistic than adding characters to a film |
I was trying to figure out a use case for input objects, and creating a new object seems like the most common use for them. But perhaps we could just skip input objects. |
Also while you're at it - take a look at some of the examples on /index.html - they're chosen for simplicity to fit on the screen, but it would be awesome if they aligned well with the schema used throughout examples. |
For input objects, how about: input ReviewInput {
# 0-5 stars
stars: Int
# what'd you think?
commentary: String
}
type Mutation {
addReview(film: ID!, review: ReviewInput!): AddReviewResult
}
type AddReviewResult {
didSucceed: Boolean!
film: Film
review: Review
} |
OK, I see that you're thinking of the schema as strictly about the movies, not as an in-universe thing (adding characters, or giving someone credits, doesn't make sense if you're thinking about it as a movie resource for fans). So the schema use case is "a schema for an app where fans can learn and interact about starwars" |
It could go either way, but we need to expand it in some sensible way to support these other featuees for ease of explanation :) |
Re IDs: Being more generally accepting of a wider range of input is one benefit, but the primary benefit is the semantic value. It distinguishes from strings you might display to a user. It also helps in migrations when you might start using Int based ids but later realize the scale problem and want to migrate to 64int or String based ids. Or when you need to unify multiple systems which use different storage types for their ids. This is a very real problem we faced at Facebook many times, and so making String/Int backing an implementation detail of the semantic ID offers more freedom for implementors and shields clients from that |
For reference, here are all the code samples from index.html:
I don't think we should update the example schema to include all of these types, but I think we can rewrite the examples to draw from a more concise schema. |
Cool. Doesn't have to be perfect. Front page should optimize for simplicity rather than consistency with the rest of the docs, but places where consistency is a no brainer should change |
OK, going to merge this for now since all of the existing examples have been converted. |
I went on a quest to design a schema that exercises every feature of GraphQL schemas I know of, and this is what I have so far.
The other goal is to reimplement the schema using the schema language directly, rather than having it in comments that are in danger of becoming outdated.