Skip to content
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

Adding allowDefaultResolve option to makeExecutableSchema to allo… #45

Merged
merged 2 commits into from May 12, 2016

Conversation

sethtippetts
Copy link
Contributor

@sethtippetts sethtippetts commented May 7, 2016

Adding allowDefaultResolve option to makeExecutableSchema to allow non-scalar types to use the GraphQL default resolve function.

Issue: #44

…w non-scalar types to use the GraphQL default resolve function
@apollo-cla
Copy link

@sethtippetts: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -48,7 +49,7 @@ function _generateSchema(

addResolveFunctionsToSchema(schema, resolveFunctions);

assertResolveFunctionsPresent(schema);
assertResolveFunctionsPresent(schema, allowDefaultResolve);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than passing in allowDefaultResolve to assertResolveFunctionsPresent, how about we just have an on/off switch? I think that will be easier for folks to understand because either they get all of the checks or they get none. There will be other ways to check arguments, and I can already foresee someone wanting to turn off checks for resolve functions present in resolvers but not in the schema. Having too many fine-grained controls may make it harder for people to understand what's really going to happen when they toggle a combination of options.

If we do that, we can just call the argument checkResolvers, and do something like this:

if (checkResolvers) {
  assertResolveFunctionsPresent(schema);
}

While we're at it, we should also pass that in to addResolveFunctionsToSchema, and turn off the checks there that make sure the types / fields defined in the resolvers actually exist.

What do you think? Happy to discuss this further!

@helfer
Copy link
Contributor

helfer commented May 10, 2016

@sethtippetts Thanks a lot for the PR, I could merge it as is, but I think we have an opportunity here to kill two birds with one stone (see in-line comment). Let me know what you think!

@helfer
Copy link
Contributor

helfer commented May 10, 2016

Talked about this a little more with @stubailo and another option would be to pass in a resolverValidationOptions parameter, which is an object:

resolverValidationOptions = {
  requireResolversForNonNull: true,
  requireResolversForArgs: true,
  rejectExtraResolvers: true,
}

@sethtippetts
Copy link
Contributor Author

sethtippetts commented May 12, 2016

@helfer I like the resolverValidationOptions a lot! I initially wrapped assertResolveFunctionsPresent in an if but thought people may still want to validate a resolver when the schema says it accepts arguments.

I've updated the PR to use resolverValidationOptions

@helfer
Copy link
Contributor

helfer commented May 12, 2016

@sethtippetts Awesome! I'll open a separate issue to keep track of the fact that we'd like to add the 'rejectExtraResolvers' option at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants