Expose getOperationRootType(schema, operationNode)

[mjmahone]

We're finding we frequently do switch statements on an OperationDefinition's operation to pull the right type out of the schema. This hardens that approach: if for some reason we added a new operation kind, you could just update this function, rather than all the callsites that match on the operation type.

[kassens]

See 1 inline comment.

[kassens]

Can you drop the ? here when you add a default case like this:

default:
  (operation: empty);
  invariant(false, 'Unexpected operation: `%s`', operation);

[IvanGoncharov]

@mjmahone I think this function having a confusing return value since you get undefined both if a schema doesn't have such operation and if a caller provides operation name that graphql-js doesn't support.

Just as an idea maybe it worth to make this function public:

graphql-js/src/execution/execute.js

Lines 418 to 424 in 2eccaad

	/**
	* Extracts the root type of the operation from the schema.
	*/
	export function getOperationRootType(
	schema: GraphQLSchema,
	operation: OperationDefinitionNode,
	): GraphQLObjectType {

It can be generalize to accept both OperationDefinitionNode and OperationTypeDefinitionNode nodes as input.
Also it's very useful that getOperationRootType throws GraphQLError binded to particular AST node.

[mjmahone]

@IvanGoncharov thank you, that looks like the exact function we actually need!

I'll probably want to move that function out of execute though: I don't think that's where it belongs, as it's useful even when we're not trying to return a value.

[IvanGoncharov]

@mjmahone Would be great to have it in utilities dir.
To make this function more generic I think this error message shouldn't mention execute:
Can only execute queries, mutations and subscriptions.

[IvanGoncharov]

You can rewrite it as:

expect(() => getOperationRootType(testSchema, doc.definitions[0]))
  .to.throw()
  .deep.equal({
    descripton: 'Schema does not define the required query root type.',
    locations: [/* */],
  });

That way your also testing that errors are binded to correct location.

[mjmahone]

That's a good suggestion, though I'm having a hard time finding other examples of this being done. I'm not really familiar with Chai, so I'm OK not being the first person to use all its best features. Additionally, this feels closer to "testing the current API and internal structure for GraphQLError" instead of "testing getOperationRootType". Basically, if we make a change to one of GraphQLError's properties, I don't want the person doing that to have to figure out why this test is failing.

[IvanGoncharov]

@mjmahone You right I run 'grep' and there are no such checks in other places.
I still think it's important to test that errors are bind to correct location.
But you right such tests shouldn't be very dependent on GraphQLError 👍

[IvanGoncharov]

@mjmahone Do you still need get-operation branch or it can be deleted?

Also please use Squash and merge to not pollute commit history with intermediate commits:

[mjmahone]

@IvanGoncharov thanks! I will make sure to squash and merge going forwards: sorry, I am relatively new to merging my own pull requests, and still learning best practices in this area. Thank you for helping enforce best practices!

[IvanGoncharov]

@mjmahone No problem. Thanks for great PR 👍