Add resolver validation, check if value is a function #669
Conversation
robzhu
changed the title
| @@ -506,6 +506,11 @@ function defineFieldMap<TSource, TContext>( | |||
| `${type.name}.${fieldName} field type must be Output Type but ` + | |||
| `got: ${String(field.type)}.` | |||
| ); | |||
| invariant( | |||
| isValidResolver(field.resolve), | |||
There was a problem hiding this comment.
you could probably just inline this as !field.resolve || typeof field.resolve === 'function'
There was a problem hiding this comment.
"!field.resolve" means we will accept any "falsey" value such as 0 or the empty string. Is that what we want?
There was a problem hiding this comment.
You're right, that it would let in falsey values. We could instead use field.resolve == null || typeof field.resolve === 'function' to be more specific
There was a problem hiding this comment.
I tried that, but the linter treats == as an error. I can update the linter to allow it if you feel strongly that == is more expressive.
There was a problem hiding this comment.
We should probably enable == null in the linter. I think eslint now has the ability to whitelist that particular kind of usage. It's equivalent to the longer form.
| @@ -506,6 +506,11 @@ function defineFieldMap<TSource, TContext>( | |||
| `${type.name}.${fieldName} field type must be Output Type but ` + | |||
| `got: ${String(field.type)}.` | |||
| ); | |||
| invariant( | |||
| isValidResolver(field.resolve), | |||
| `${type.name}.${fieldName} field resolver must be function or null/` + | |||
There was a problem hiding this comment.
how about field resolver must be a function if provided, but got:
|
btw - this looks good - you should merge once you've got something you think is best |
If a field's resolver is set to a constant value like "{}" we get a runtime error, "resolveFn is not a function". We can easily detect this case when we validate fields during schema creation. This PR adds a new validation rule to check that the resolve definition is a function.