Add directive support #225
Conversation
| case InputValue.ObjectValue(fields) => | ||
| fields.map { case (key, value) => s"$key: ${renderDirectiveArgument(value)}" }.mkString("{", ",", "}") | ||
| case InputValue.VariableValue(_) => | ||
| throw new Exception("Variable values are not allowed in a directive declaration") |
There was a problem hiding this comment.
I'd rather not render it here than throwing an exception.
There was a problem hiding this comment.
Checking the spec after you mentioned this I realized there are a couple more things to be mindful of: https://spec.graphql.org/June2018/#sec-Type-System.Directives
Validation
A directive definition must not contain the use of a directive which references itself directly.
A directive definition must not contain the use of a directive which references itself indirectly by referencing a Type or Directive which transitively includes a reference to this directive.
The directive must not have a name which begins with the characters__(two underscores).
For each argument of the directive:
The argument must not have a name which begins with the characters__(two underscores).
The argument must accept a type where IsInputType(argumentType) returns true.
There was a problem hiding this comment.
I don't know if it would be possible to resolve these at compile time, so it probably would have to be a runtime error, but the question is how should we handle it? Agree that throwing isn't the best option, should we have two rendering methods? one that simply suppresses errors and one that returns an IO[ValidationError, String]
There was a problem hiding this comment.
This is a bit similar to issue #170 where we lack a way to report something abnormal within the schema. Ideally we should fail at the point of building the schema rather than later during query execution or rendering like here.
It bothers me to make graphQL(...) return an effect because attaching the wrappers with @@ wouldn't be as smooth as it is now, but how about making .interpreter return an effect? That way we can build the schema easily, but when we turn it into an interpreter (which we do only once), we verify that the schema is valid and fail if it's not. What do you think?
There was a problem hiding this comment.
Btw if we go for that solution, we can do it in a later PR and just remove the check for now (so that you don't need to implement the whole thing to get this PR merged).
There was a problem hiding this comment.
.interpreter return an effect
I think this makes sense, since it is the "effect" of becoming an interpretable schema.
Ok I'm fixing so that it will just remove the arguments if they aren't valid and then we can fix it with the change you mentioned above.
|
This is really nice! Additionally can you mention in the docs:
|
Revamped version of #192
This adds schema directives which are needed for such extensions as cache-control and federation. In order to test the behavior I also included an implementation of the cache-control extension using directives and the new wrappers.