Better Predicates #1137
Better Predicates #1137
Conversation
leebyron
force-pushed
the
better-predicates
branch
3 times, most recently
from
b748679
to
5df345b
Compare
Introduces new predicate and assertion functions for each kind of type mirroring the existing ones for the higher order types. It also includes predicates for directives and schema. This should remove the need to use `instanceof` in any usage of GraphQL.js This consolidates the checking of instances which generalizes the cross-realm/multiple-module check so that the clearer error message is provided in nearly all scenarios, reducing confusion. This also replaces nearly all `instanceof` with new predicate functions internally. Fixes #1134
leebyron
force-pushed
the
better-predicates
branch
from
5df345b
to
a24b300
Compare
| throw new Error( | ||
| `Cannot use ${constructor.name} "${value}" from another module or realm. | ||
|
|
||
| Ensure that there is only one instance of "graphql" in the node_modules |
There was a problem hiding this comment.
@leebyron Not sure if anybody still using alternative package managers(e.g. bower) but maybe it worth to prefix this string with "If you are using NPM package" to reduce possible confusion.
| export function assertNonNullType(type: mixed): GraphQLNonNull<any> { | ||
| invariant( | ||
| isNonNullType(type), | ||
| `Expected ${String(type)} to be a GraphQL Non-Null type.`, |
There was a problem hiding this comment.
@leebyron Maybe it worth to say something like to be wrapped in?
Same goes for List.
| return type; | ||
| } | ||
|
|
||
| declare function isNonNullType(type: mixed): boolean %checks(type instanceof |
There was a problem hiding this comment.
@leebyron Since antagonist function called isNullableType maybe it worth to rename this one to isNonNullableType?
| export function assertWrappingType(type: mixed): GraphQLWrappingType { | ||
| invariant( | ||
| isWrappingType(type), | ||
| `Expected ${String(type)} to be a GraphQL wrapping type.`, |
There was a problem hiding this comment.
@leebyron Since this error messages is intended for developers who just use public API they can be unfamiliar with what wrapping type means. Maybe it worth to replace it with something like to be a type wrapped by either NonNull or List wrapper?
Similarly, other error messages will benefit from replacing composite terms like leaf type, abstract type, composite type, etc.
| type instanceof GraphQLScalarType || type instanceof GraphQLEnumType, | ||
| 'Must provide Input Type, cannot use: ' + String(type), | ||
| ); | ||
| if (isScalarType(type) || isEnumType(type)) { |
There was a problem hiding this comment.
@leebyron Can it be replaced with isLeafType?
Introduces new predicate and assertion functions for each kind of type mirroring the existing ones for the higher order types. It also includes predicates for directives and schema. This should remove the need to use
instanceofin any usage of GraphQL.jsThis consolidates the checking of instances which generalizes the cross-realm/multiple-module check so that the clearer error message is provided in nearly all scenarios, reducing confusion.
This also replaces nearly all
instanceofwith new predicate functions internally.Fixes #1134
Closes #1016