Remove Interface is implemented by 1+ Objects validation

[mjmahone]

This validation requirement was a really nasty breaking change for existing GraphQL type systems. Additionally, it is in my opinion actively harmful to an iterative development model, where you'd define an interface that a field returns, develop on the client against that interface, then days or weeks later, create one or more types that implement the interface, which old clients can start using immediately.

[IvanGoncharov]

develop on the client against that interface

How can you develop against an interface if it's not implemented?
Do you use some mocking tool?
What would be the value of __typename for a field that returns such interface?

[mjmahone]

At least internally, we have a lot of tooling that generates client-side, language-specific models. So for Relay, we have flow types for fragments that you define. How we (frequently) do this:

1. Create the interface definition with the fields you'll want to support on that interface.
2. Add a field to another type which returns that interface. On the server, the implementation always returns null.
3. Build your client using the generated flow types and potentially client-defined dummy data.
4. Implement the interface with a specific object, have the field you defined in (2) return that object.

Because GraphQL schemas are meant to be forwards compatible, all of our clients have the concept of an "unknown implementation of Interface Foo" built into them, with the assumption that we'll get objects with __typename we don't know, but because we've built our fragments against the Interface and not a specific object, it's OK.

[OlegIlyenko]

It is interesting to see this PR, I also had mixed feelings about this validation. In my case ita breaking change as well.

In our case we provide a multi-tenant system where most of the schema is static, but small parts of the schema are specific to the tenant and generated dynamically for each project (it is a set of custom product attributes). All tenant-defined parts of the schema implement a common interface - it is advantageous to consider the interface as a part of the static schema and assume that it is always present for all tenants. On the other hand, user-defined types (the implementations of this interface) might or might not be present. So it is possible that some tenants end up in a situation where they have a non-implemented interface. Though, it is still useful to have this interface in the schema for client-side tooling.

For the moment being, we disabled the validation in our project. I believe it would be possible to refactor the schema, but I think I would prefer to avoid it. In general, I don't mind this validation as log as it is not prescribed by the GraphQL spec. I see it more as helper that specific implementation might adopt to help users avoid common schema configuration issues. As long as there is an easy way to disable the validation and it is not part of the spec, I think it should be fine.

[mjmahone]

As this validation will not be included in graphql-js v14.0.0, I'm going to merge this and put up a new PR for adding this validation back in. I do not want our spec and reference implementations getting out of date.

I'm still very open to adding this validation. But because it's a major breaking change, we need more discussion on the value of this enforcement vs. allowing un-implemented interfaces.

[mjmahone]

New PR with RFC: #460

[IvanGoncharov]

@mjmahone It's a draft version of a specification so it's normal that it deviates from graphql-js. Plus it safe do any changes until a new release is published.

I agree that it makes sense to prove that such validation doesn't cause more harm than good. And I 100% agree that it worth to exclude this validation from upcoming release if no good solution found for both your and @OlegIlyenko use cases.

But I don't understand what is a reason for such a quick merge. It was worth to wait at least a day to hear what others have to say especially the @mattkrick (the author of original PR) and @leebyron (the person who merged that PR).

I'm totally against merging such non-trivial PRs without giving the community some time for feedback. As far as I remember previously all non-editorial changes were opened for discussion for more than 3 hours.

[mjmahone]

@IvanGoncharov sorry about that. I was under the impression that graphql-js and graphql needed to stay in sync as quickly as possible. Which meant in my mind, as soon as it became obvious that 14.0.0 wasn't going to include this spec addition, we should get rid of it rather than letting the two drift.

This is my error: had I realized it was OK for the spec and reference implementation to be out of sync for a few days, I would have happily left this un-merged. I'm sorry, and will allow more time for comments from the community going forward. Of note: prior to merging this I had reached out to @leebyron already, and he OK'd the graphql-js strategy. I likely misunderstood what he wanted us to do for the spec, though.

[IvanGoncharov]

@mjmahone Thanks for the explanation 👍
Nothing bad happened but I think it's a good idea to have some kind contribution guide that explains what to do in such situations.
Also if possible keep us in sync with Facebook internal discussions so we are all on the same page.

[leebyron]

Hey all, sorry for the delayed response. I was actually about to add a comment with support and work on the merge, but I see @mjmahone already pushed forward - we had spoken about a plan for this PR offline first.

Since a primary goal in future editions of the spec is to limit breaking previously spec-complaint implementations, and that this particular change caused issue with both Facebook's implementation and @OlegIlyenko's - I think that is merit enough to roll back the change. I also think there are likely to be cases during active development of a GraphQL API where an interface is used as a placeholder describing future types which do not yet exist - a legitimate scenario where 0 implementing types could still result in a working schema.

Because we've been talking in the WG sessions about cutting the spec release, testing release candidates of GraphQL.js was a priority which helped expose this issue - so merging this in an expedited way makes sense, we'd rather not introduce a new rule in the next released spec we knew was problematic.

Additionally, we knew that some variation of this rule might still make sense and wanted to leave open discussion around re-introducing the rule to provide value while limiting the identified costs. That was the reason for opening #460

[mattkrick]

good call. the important part (getPossibleTypes returning an empty array) was not rolled back in the reference implementation, so I'm cool with this. The multitenancy use case is a fun one! Certainly makes the case for favoring 0-to-many over 1-to-many.

So if it's not an error, is it a warning? info? Is there a utility function that I can use to find unused interfaces?