-
-
Notifications
You must be signed in to change notification settings - Fork 809
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bit confused on getDirectiveDeclaration #1022
Comments
Just tested and doesnt appear to be the multiple declaration thing as it only ever executes once and a prev declaration (the one made in my file) is always present - so this example seems to be incorrect and dated since the final branch that declares a new directive will never occur as an error will happen if the schema did not already declare the directive. |
I'm running into the same issue. Any new about that? |
I have the same problem and was able to find this comment from graphql-tools issue #957. I currently believe
|
Is this solved? I just tried and the function is not called... |
This should be fixable by adding in typedefs from getDirectiveDeclaration prior to calling buildSchema. In general, the declaration might depend on additional input types, but support for that can be added later. The latter is akin to adding true schema merging capability into makeExecutableSchema (as opposed to the stitching/proxying done by the confusingly named mergeSchema). Folding into #1306 |
Moving to v5.1, reopening to move any relevant discussion here. |
Hi! Closing this issue for now as within v6, as the difficulty is not that getDirectiveDeclaration is not working, but just that graphql-js v14 and above, buildSchema requires directives to be declared. One can reverse this requirement by telling buildSchema to assume the SDL is invalid or passing custom rules, but this seems like a lot of rearchitecting when existing workarounds are not too bad:
Rather than changing makeExecutableSchema to act more like stitchSchemas, users should be able to specifically choose which approach to take. |
@yaacovCR Could you please document that somewhere (maybe in the readme or directly in the code), because I would never have known as a regular user if I was not watching this issue and its a very confusing thing currently. |
Good idea! I believe #1463 documents it for the newer schema transforms. We can definitely accept PRs to clarify retained legacy class based schema directive documentation around this, and after v6 lands would be a good time to fill in docs in general. Maybe you could create a separate issue after v6 lands to track whatever docs deficiency there still is... |
So according to the documentation it seems to make that by defining the
static getDirectiveDeclaration
it would mean that the directive should work even if the schema does not declare it at all.This appears to be untrue and the example appears to be pointless for the most part. I am not sure I understand any part of the purpose of it. I am sure I am just missing a point.
First of all, I see the reasoning why a valid declaration would be
required
and that something likegetDirectiveDeclaration
would be used simply to validate the users declared directive and throw an error during startup should it be invalid.However, according to the documentation in the example:
This makes it seem like if the directive were not declared in the
typeDefs
that it would allow constructing a new directive that would be used in this case. This is not true and doesn't work.when
Which in this case I am trying to log and just play with the idea, in practice I believe it makes more sense to allow for this but for this to be strictly used to validate the user declared it properly.
I guess with how it works now (only working if the directive is actually declared) means I can't see why the last part of this example where a new directive is built makes any sense at all to exist since the previous directive should ALWAYS exist?
In addition I can't see how this is a good idea:
Overwriting the declared type silently and reducing auth permissions without the user knowing sounds like a terrible idea.
Is this only meant to be used in the case there are cascading duplicate declarations of the directive when something like merge is being doing? This would make a lot more sense but isn't defined too clearly in the docs if it is the case.
The text was updated successfully, but these errors were encountered: