Add resolver validation options for Interface and Union types

[reconbot]

TODO:

• If this PR is a new feature, reference an issue where a consensus about the design was reached.

No consensus yet, but this is a precursor to closing #616 and removes an internal TODO. Exact validation options are up for debate.

• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change. Include a description of your change, link to PR (always) and issue (if applicable). Add your CHANGELOG entry under vNEXT. Do not create a new version number for your change yourself.

• requireResolversForResolveType will require a resolveType() method for Interface and Union types. This can be passed in with the field resolvers as __resolveType()

This doesn't do anything on graphql@0.11 because it provides a default resolveType. That's ok.

[benjamn]

What if we inverted the option so that a missing resolveType caused a warning by default, but you could pass resolverValidationOptions.allowMissingResolveType (or something like that) to disable the warning?

I'm worried no one is going to know they should start passing requireResolversForResolveType: true, so they won't ever see this warning.

[reconbot]

I'd much rather throw than log because logging in a production can be complated. This library has examples of both. I think all validations should be on by default but that's a major change.

…

On Wed, Mar 28, 2018, 6:56 PM Ben Newman ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/schemaGenerator.ts <#698 (comment)> : > @@ -460,6 +463,24 @@ function addResolveFunctionsToSchema( } }); }); + + // If we have any union or interface types throw if no there is no resolveType or isTypeOf resolvers + if (requireResolversForResolveType) { + checkForResolveTypeResolver(schema); What if we inverted the option so that a missing resolveType caused a warning, but you could pass resolverValidationOptions.allowMissingResolveType (or something like that) to disable the warning? I'm worried no one is going to know they should start passing requireResolversForResolveType: true, so they won't ever see this warning. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#698 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlbudittDHFKGe7slJ2BGgZCMeAdYVks5tjBUrgaJpZM4S-vzu> .

[benjamn]

What if passing requireResolversForResolveType makes it throw on failure (like this PR does), but you get a warning (at least in development?) if you don't pass that option, and the only way to silence the error is to pass requireResolversForResolveType: false?

[reconbot]

That's an interesting pattern. I kind of like it.

…

On Wed, Mar 28, 2018, 7:15 PM Ben Newman ***@***.***> wrote: What if passing requireResolversForResolveType makes it throw on failure (like this PR does), but you get a warning (at least in development?) if you don't pass that option, and the only way to silence the error is to pass requireResolversForResolveType: false? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#698 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABlbtkxZc0R5sDIcEx1xxX6jGjoKMPKks5tjBmegaJpZM4S-vzu> .

[reconbot]

How's this? I added the warning, unsurprisingly it spams the test suite a bit.

[stubailo]

For what it's worth, when we used to have all the validations turned on back in the day but they felt restricting to many (most?) people.

[reconbot]

rebased off of master and squashed my commits, lmk if there's anything else that needs to happen on this as I want to get working on #616

[zfoster]

Very interested in this getting merged in. Is this now just waiting on conflicts being resolved?

[reconbot]

I've rebased the changes again.

[vepanimas]

@reconbot @benjamn what if all types that implement an interface have their own isTypeof method? It will generate an error but IMHO it should not because all types are correctly resolved at runtime.