Skip to content

Add failing test in findBreakingChanges-test.js #2711

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

Closed

Conversation

DrewML
Copy link

@DrewML DrewML commented Jul 15, 2020

Not sure if I'm overlooking some cases, so I'm starting with a failing test to help demonstrate the proposed change.

The output of the test in this PR is:

1) findBreakingChanges
    should not flag args changing from String or Int to ID as breaking:

    AssertionError: expected [ Array(2) ] to deeply equal []
    + expected - actual

    -[
    -  {
    -    "description": "Type1.field1 arg arg1 has changed type from String to ID."
    -    "type": "ARG_CHANGED_KIND"
    -  }
    -  {
    -    "description": "Type1.field2 arg arg1 has changed type from Int to ID."
    -    "type": "ARG_CHANGED_KIND"
    -  }
    -]
    +[]

Looking through the commit history of findBreakingChanges, it seems like the feature was designed to follow the spirit of "breaking changes" as mentioned in the spec:

As GraphQL type system schema evolve over time by adding new types and new fields, it is possible that a request which was previously valid could later become invalid. Any change that can cause a previously valid request to become invalid is considered a breaking change

If an argument's type is changed from either a String or Int to ID, a query providing either of those input types should still pass validation, and be capable of executing successfully (assuming the field resolve functions adjusts for input coercion).

@IvanGoncharov
Copy link
Member

@DrewML It's breaking for the case where your clients pass it as query variable since it's explicitly marked as String:

query ($id: String) {
  field1(arg1: $id)
}

changing it to string will break this rule
http://spec.graphql.org/draft/#sel-IALbLDHCJCAACIBohU

Validation failures occur when variables are used in the context of types that are complete mismatches, or if a nullable type in a variable is passed to a non‐null argument type.

For context it was discussion about changing the spec to allow this use case:
graphql/graphql-spec#526

@DrewML
Copy link
Author

DrewML commented Jul 15, 2020

Thanks @IvanGoncharov! I definitely overlooked that 😄. Makes total sense though.

@DrewML DrewML closed this Jul 15, 2020
@DrewML DrewML deleted the findBreakingChanges-id-input-type branch July 15, 2020 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants