Use invariants instead of throwing Errors #1457
Conversation
| @@ -116,9 +117,7 @@ export function buildASTSchema( | |||
| ast: DocumentNode, | |||
| options?: BuildSchemaOptions, | |||
| ): GraphQLSchema { | |||
| if (!ast || ast.kind !== Kind.DOCUMENT) { | |||
| throw new Error('Must provide a document ast.'); | |||
| } | |||
There was a problem hiding this comment.
Make it similar to this:
graphql-js/src/utilities/extendSchema.js
Lines 105 to 108 in 3fdf240
IvanGoncharov
force-pushed
the
invariantsInsteadOfThrow
branch
from
c05f897 to
c5b7990
Compare
src/utilities/buildASTSchema.js
Outdated
| @@ -342,7 +344,7 @@ export class ASTDefinitionBuilder { | |||
| case Kind.INPUT_OBJECT_TYPE_DEFINITION: | |||
| return this._makeInputObjectDef(def); | |||
| default: | |||
| throw new Error(`Type kind "${def.kind}" not supported.`); | |||
| invariant(false, `Type kind "${def.kind}" not supported.`); | |||
There was a problem hiding this comment.
While the above case should probably be invariant, this one should almost definitely me throw new GraphQLError: you've got a type definition that somehow parsed but at validate time is invalid. In this instance it's very helpful to see where in your document that error actually is.
There was a problem hiding this comment.
@mjmahone You right 👍 especially tanking into account possible Experimental features.
IvanGoncharov
force-pushed
the
invariantsInsteadOfThrow
branch
from
c5b7990 to
22ec55b
Compare
There was a problem hiding this comment.
Approving, but we should also fix the throw Error to throw new GraphQLError. In general, if we have an AST node we should be throwing GraphQLError instead of Error or invariant, unless the issue is caused by tooling built on top of graphql-js making a mistake, rather than someone using a GraphQL document that won't work.
No description provided.