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

Best Practices : Versioning GraphQL APIs #884

Open
wants to merge 8 commits into
base: source
Choose a base branch
from

Conversation

nisarg1499
Copy link

I have added Versioning of GraphQL APIs under Best Practices section.
Topics added in the writing :

  • Versioning of REST APIs
  • Versioning GraphQL APIs
  • Example of Shopify : how they have versioned their GraphQL APIs
  • Some more info and tools for schema validation(useful for versioning)

Mentor for this writing : @Urigo

@carolstran
Copy link
Member

👋🏼 Looping this PR into the discussion going on #41. That way, if people find their way there through the GSoD proposals then they'll know that you're already working on this one 😊

@nisarg1499
Copy link
Author

👋🏼 Looping this PR into the discussion going on #41. That way, if people find their way there through the GSoD proposals then they'll know that you're already working on this one 😊

@carolstran Can you review this PR? The PR is ready for review.

name: String!
Username : String!
birthDate : DateTime!
lastLoginHistory : NewDateTime!
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but... this would be a breaking change for clients?

Copy link
Author

@nisarg1499 nisarg1499 Mar 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekampf How? I think we can evolve schema by referencing schemas to others. Any specifics why this will be a breaking change?

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Searching for "deprecated", "deprecate" and "deprecation" with the graphql.org search bar doesn't have any matches so it's clear that we do need some content like this; so thanks for opening the conversation 👍 However, I don't think we should add a section called "Versioning" as versioning does go against the generally agreed best practices of GraphQL.

I think we could take a slightly different tact and talk more about deprecation, explaining what changes are allowed to be made without "breaking" the schema, and maybe note that this approach which avoids explicit versioning allows your schema to evolve over time without ever breaking older clients/queries.

There's been extensive discussion about Shopify's versioning pattern in recent GraphQL Spec WGs; here's some relevant notes if you're interested (you can also view the YouTube recording): https://github.com/graphql/graphql-wg/blob/c3dda0548c8b7114ee693bf7334ba714575dce70/notes/2020-09-03.md#allowing-deprecation-of-inputs-10m-evan (in general the GraphQL Spec WG seem reluctant to encourage this form of versioning)

The examples in this PR use firstName, middleName and lastName which really aren't universal concepts and I'm very hesitant to add them to the GraphQL documentation; read more here: https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/ Can you come up with an example that is more universal?

Comment on lines 31 to 33
id: Int!,
firstName : String!,
lastName : String!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for trailing commas, and the whitespace is inconsistent. May I recommend piping this file through prettier --prose-wrap always to automatically format the markdown (including automatically formatting the GraphQL snippets).

Suggested change
id: Int!,
firstName : String!,
lastName : String!
id: Int!
firstName: String!
lastName: String!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll reformat the file through prettier in VS code. I hope that will fix all format issues.

Comment on lines 40 to 43
Id : Int!,
firstName : String!,
middleName : String!
lastName : String,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few formatting issues, id became capitalised, and lastName becoming nullable was a breaking change.

Suggested change
Id : Int!,
firstName : String!,
middleName : String!
lastName : String,
id: Int!
firstName: String!
middleName: String
lastName: String!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It was just a typo by me. Fixed in next commit.

}
```

Here we have added the middleName parameter which is mandatory and have changed lastName to optional.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can change an optional field to required, but you cannot change a required field to optional (that's a breaking change). The term field is the correct term to use here (rather than parameter).

Suggested change
Here we have added the middleName parameter which is mandatory and have changed lastName to optional.
Here we have added the `middleName` field which is optional.

Here we have added the middleName parameter which is mandatory and have changed lastName to optional.


* The best practice is to use “@deprecated(reason)” defined in the schema. By using this the client can know which filed to use and which one is deprecated. This will not define the version number as the semantic versioning does. The deprecated fields will be shown by the GraphQL Explorer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wording of this bullet needs revisiting. Separately, I think it's probably better for it to be a section title such as "The 'deprecated' directive" rather than a bullet.

Comment on lines 53 to 65
### More info on Versioning
There are few other best practices for versioning GraphQL APIs.
Version constraints can be added to the query which will tell that the field will be resolved by which version. Using version constraints we can access different fields of different versions. For example, the query will look like :
```graphql
query {
account(id : 5){
old : profilePhoto(versionContraint : “^0.1”)
new : profilePhoto(versionContraint : “^0.2”)
}
}
```

We can define the versions in the schema and query can be written as above to access the fields of different versions.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen this recommended as a best practice before (and am very dubious); could you share details of where you've seen this recommended as a best practice? Also typographic quotes have slipped in where you meant straight quotes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually tagging+semantic versioning. I do not remember where I read this, but I'll try to find it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjie Should I keep this point?

@nisarg1499
Copy link
Author

@benjie Thanks for reviewing. I will look into your comments and make changes wherever required.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Feb 27, 2021

CLA Signed

The committers are authorized under a signed CLA.

@nisarg1499
Copy link
Author

@benjie Sorry for the delay. In a couple of days I will try to make all suggested changes and then you can review it.

@nisarg1499
Copy link
Author

The examples in this PR use firstName, middleName and lastName which really aren't universal concepts and I'm very hesitant to add them to the GraphQL documentation; read more here: https://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/ Can you come up with an example that is more universal?

@benjie Do you have any ideas/examples which we can added instead of firstName and lastName? Actually I added considering profile section in mind. Most of the products have profile sections. That's why.

@nisarg1499
Copy link
Author

There's been extensive discussion about Shopify's versioning pattern in recent GraphQL Spec WGs; here's some relevant notes if you're interested (you can also view the YouTube recording):

@benjie Can you share the youtube link? I tried searching but was not able to find it. I'll capture main points from video and add in the page.

@benjie
Copy link
Member

benjie commented Mar 15, 2021

@nisarg1499 Sure; videos are here:

https://www.youtube.com/channel/UCERcwLeheOXp_u61jEXxHMA

Notes are here:

https://github.com/graphql/graphql-wg/tree/main/notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants