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

Optional to Required should be backwards compatible #9

Closed
danpalmer opened this issue Feb 5, 2019 · 4 comments
Closed

Optional to Required should be backwards compatible #9

danpalmer opened this issue Feb 5, 2019 · 4 comments

Comments

@danpalmer
Copy link

I've just seen an issue where a backwards incompatible change has been detected, when a field went from optional to required in the API output.

I'm not sure that this should be backwards incompatible?

Field type changed on field User.cart from : `"Cart"` to `"Cart!"`.

On the wire, this will be either null or an object. Clients using checks or even codegen may be currently supporting this as optional (i.e. with a Maybe(Cart) type), but given that the format on the wire looks identical in the required case, I think it's fully backwards compatible.

Would be keen to understand if this is not backwards compatible in the general case, or get a fix out if it is.

@jarwol
Copy link
Owner

jarwol commented Feb 7, 2019

I agree that the reverse case is backward compatible (required field becoming optional), changing a field from optional to required still strikes me as a breaking change. Producers of the data being requested by GraphQL could happily produce your User object completely without a cart field when it has type Cart, but that data is no longer valid once the type is changed to Cart!.

@danpalmer
Copy link
Author

@jarwol thanks for the reply here, sorry it has taken me so long to come back to this.

I'm not sure if I fully understand? From my reading of your comment, you seem to be suggesting that data "upstream" from the API, say in the database, could become invalid? I agree that could indeed happen, but I don't think the GraphQL type system should try to impose itself on systems that do not interact with it.

If a server changes the type from optional to required it is saying that its API contract is that it will return something. How it does that, how it decides what user object to build, how it figures out what to return, those are all internal implementation details that shouldn't be a concern of the API contract.

If I've misunderstood something please let me know!

@jarwol jarwol closed this as completed in 1152adb Mar 20, 2019
@jarwol
Copy link
Owner

jarwol commented Mar 20, 2019

You make a good point. Backwards compatibility should only take downstream consumers into account. Fix is out. Thanks for the report!

@danpalmer
Copy link
Author

Thanks! Great to see all these fixes!

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

2 participants