Detect new interface implementation for new types #40
Conversation
| if type.graphql_definition.is_a?(GraphQL::ObjectType) | ||
| type.interfaces.each do |interface| | ||
| if old_types[interface.graphql_name] | ||
| changes << Changes::ObjectTypeInterfaceAdded.new(interface, type) |
There was a problem hiding this comment.
I guess ideally this would only be dangerous for existing types right? It's good to capture this change for new types, but it's not actually dangerous at this point.
There was a problem hiding this comment.
Exactly. It's only dangerous when implementing an existing interface.
|
This is actually a subset of #33. Ideally we'd capture every change for a new type? |
|
🤔 You're right there's definitely overlap between this issue and #33 but do you have any insight into the use cases where reporting nested changes would be generally useful? My main concern is applying a recursive strategy more broadly could result in a large number of changes being reported making it harder for developers to understand the root cause of lots of warnings. For example when a type is removed, the recursive strategy would report a change for the type's removal, a change for each of its fields being removed, and a change for each its fields' arguments being removed. Perhaps we could return a tree of changes (e.g. the type removed change node would have child change nodes for each of the removed type's fields) and leave it up to clients how they want to process that tree (e.g. whether or not to stop the recursion when they hit the first breaking change node)? I'm just not sure it buys a client much over walking the graphql-ruby types though. On a somewhat related note, I'm beginning to wonder if removing a type is actually a breaking change. It seems like the real breakage is from removing fields on other types that could break selection sets or removing an interface implementation that could break fragments. Admittedly this is probably biased by my use case of a GitHub Action that generates annotations with links to details on impacted clients for any breaking/dangerous changes though. |
|
Our use case is a GitHub bot which ran linting on PRs. We defined hooks based on each type of change (ie: Your proposal is basically what we did internally in our bot app so yeah clients should be able to decide what they care about or not.
This sort of mirrors how deprecations work since you don't actually deprecate a type, just the fields that return them. I guess there's some nuance since removing a type can mean:
So imo it's still a breaking change to remove a type. |
This PR fixes a bug where dangerous new interface implementations were not detected for new types. While I was in here, I changed the kitchen sink test to compare strings rather than arrays of strings so test failures would print a nice diff.