Flag for treating adding union and enum members as breaking with conditional breaking changes

[n1ru4l]

Report by customer:

In our system, some changes that aren't technically breaking, like adding to an existing enum or union value, can cause issues in our downstream services.

[stephenh]

Hi all! Just adding some rationale for this...

We have several TypeScript frontends that pull in our GraphQL schema, and when we have enums, we prefer to use "exhaustive enum checks", i.e. things like:

enum Color {
  Red = "red",
  Green = "green",
  Blue = "blue",
}

function assertNever(value: never): never {
  throw new Error(`Unhandled case: ${value}`);
}

function getColorMessage(color: Color): string {
  if (color === Color.Red) {
    return "You selected Red!";
  } else if (color === Color.Green) {
    return "You selected Green!";
  } else if (color === Color.Blue) {
    return "You selected Blue!";
  }

  return assertNever(color); // Ensures exhaustiveness checking
}

// Example usage
const myColor: Color = Color.Red;
console.log(getColorMessage(myColor));

And so when our backends add a new enum value, this is not technically breaking from a wire protocol perspective, but it does risk causing both compile time errors (when the frontends pull in the next version of the GQL schema) & and also runtime errors (the throw new Error(handled)) in existing clients.

Given the severity of both of these issues, we'd like to use graphql-hive/console to track these as breaking changes. Thanks!

[jdolle]

I'll be taking on this work. We have a few options for how to prevent this issue, but the proposed approach of requiring schema approval is the most intuitive for now.

[jdolle]

After reviewing the source code and possible conditions that could cause issues, I've found overlap with the "dangerous" changes flagged by the graphql-inspector logic.

I think it'd be best to make the option "Require approval for dangerous changes". So it will encompass more than just enums. I think this is the safest approach and lets us avoid duplicating a lot of the logic that's done by the inspector already.

[blimmer]

That sounds like an excellent solution to this problem. Offloading the behavior to the "dangerous" rules upstream sounds like a very reasonable option 👍

[jdolle]

I've completed work on this feature. It will go out with our next release. Please reach out if this doesn't meet your expectations or if you have other ideas for improvements :)

[stephenh]

Amazing! Thank you @jdolle !

[blimmer]

We tested this today and it works as expected! It was so easy to set up 🎉

I've got a few pieces of feedback on this @jdolle

GitHub Action Check Report

I did notice a small issue with the GitHub Action check report, which I filed #6696 for.

Documentation

I was a little bit confused by the docs near the toggle switch:

Before enabling this feature, be sure "contextId" is used on schema checks.

I wasn't sure what that meant. My GitHub action just looks like this:

- name: Submit Schema Check to Hive
  run: |
    yarn hive schema:check 'schema/**/*.graphql' \
      --registry.accessToken '${{ secrets.HIVE_TOKEN_DEV }}' \
      --github

The linked docs (clicking "Learn More") don't specify anything about contextId either.

I ended up finding https://the-guild.dev/graphql/hive/docs/api-reference/cli#check-a-schema which says:

If you want to be able to leverage breaking change approvals, you must provide the --contextId parameter. Using --contextId is optional when using GitHub repositories and actions with the --github flag.

So the --github flag already provides this. The documentation might be improved to mention that directly.

Thank You!

Again, thank you so much for adding this. When we initially filed the support ticket requesting this feature, I thought it would be a long shot. I am thoroughly impressed with how fast y'all delivered this 🎁 ✨