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

Adding an enum value is breaking change? #968

Closed
excitement-engineer opened this issue Jul 25, 2017 · 5 comments
Closed

Adding an enum value is breaking change? #968

excitement-engineer opened this issue Jul 25, 2017 · 5 comments

Comments

@excitement-engineer
Copy link
Contributor

@excitement-engineer excitement-engineer commented Jul 25, 2017

I was wondering if adding a new value to an existing enum is considered a breaking change. The method findBreakingChanges method currently does not consider this to be a breaking change. If any clients break because of this addition does it mean that the clients implemented the GraphQL schema incorrectly? I was wondering if this is a deliberate omission in the findBreakingChanges method.

@rmosolgo

This comment has been minimized.

Copy link

@rmosolgo rmosolgo commented Jul 25, 2017

I didn't quite understand the question at first so I thought I'd share my thoughts:

Adding an enum value isn't a breaking change in purely GraphQL terms: by adding an enum value on the schema, all existing queries will still run. Enum values hard-coded in those queries are still valid, even if they're from the subset of previous values.

I guess it could break client code. For example, if you have a hard-coded set of values, and you receive something from the server which is not in that set of values, it could crash the client app.

So:

  • Should adding an enum value be a breaking change? OR
  • Should all GraphQL clients have the assumption that current enum values are a subset of possible enum values?
@excitement-engineer

This comment has been minimized.

Copy link
Contributor Author

@excitement-engineer excitement-engineer commented Jul 25, 2017

Thanks a lot for your help!

As you say it is technically not a breaking change because the query will still run. However, it is still quite scary to add an extra enum state because clients can break if they simply assumed that the enum set is fixed. Unlike adding a new field to a schema, it is not as clear what the effects will be on the clients that depend on the API.

Have you run into a project where you have had to extend an enum with additional values? If yes, did this cause any major issues?

@dschafer @leebyron I am also curious to hear if enums have been extended over time in Facebook's schema and how this was managed. Have you ever run into these issues?

@leebyron

This comment has been minimized.

Copy link
Collaborator

@leebyron leebyron commented Jul 25, 2017

It's not a schema-breaking change for the reasons @rmosolgo mentioned above. All existing queries continue to behave correctly.

It is an example of expanding the possible values a query could return. Other examples of this are adding a type to a union or adding an interface to a type. When building clients, it's best to be defensive against possible future expansions and handle those cases. For enums, we typically include an else or switch default clause when branching on them to handle cases the client doesn't know about. If your clients aren't programming defensively like this, then it's true that expanding the possible response values could cause issues for those clients - it's not an entirely safe change.

At Facebook, we've encountered some minor issues in the past with expanding enum values - but good testing has helped avoid any serious issues.

If you're worried about this, I'd suggest an audit of your own code or just communicating the upcoming change clearly first.

@leebyron leebyron closed this Jul 25, 2017
@wincent

This comment has been minimized.

Copy link
Contributor

@wincent wincent commented Jul 25, 2017

I wonder if we should consider adding this to the (potentially) "dangerous changes" list?

@excitement-engineer

This comment has been minimized.

Copy link
Contributor Author

@excitement-engineer excitement-engineer commented Jul 25, 2017

Thanks a lot for your help @rmosolgo and @leebyron. This clarifies a lot!

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

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.