Repository navigation
Did PR #4229 go too far? #4259
Description
PR #4229 unifies the behavior of all built-in directives, where in the past skip and include was treated differently from other built ins. In the process, the GraphQLSchema.Builder.clearDirectives method was removed, because this was one of the places where different built-ins were being treated differently.
However, we have a use-case where we use GraphQLSchema.transform to transform all of the non-built-in directives in a schema. This works by taking all the existing non-graphql-builtin directives from the source schema, then calling clearDirectives on the builder for the new schema, then adding transformations of the original directives into the new schema. So a semantics of clearDirectives which is "clear all non-graphql-builtin directives" would be helpful. So perhaps the change in #4229 should've been: "clearDirectives clears non-graphql built-in directives, but treats all graphql built-ins uniformly (contra the old behavior)" -- rather than getting rid of it altogether.
Activity
andimarek commentedon Feb 23, 2026
Yes I think we can add it again but preventing built in scalars to be removed.
rstata commentedon Feb 24, 2026
i'm thinking that it should delete all directives including built-ins. users are then free to add in their own version of the built-ins, and
ensureBuiltInDirectiveswill add in any that they miss. this would mimic the behavior they experience with a newly-createdBuilder.andimarek commentedon Feb 24, 2026
I am not sure about allowing users to remove built in directives.
I don't think we really support that intentionally, maybe by accident.
Do you have a specific reason for removing built ins?
rstata commentedon Feb 24, 2026
as i understand it, the way a "fresh" schema builder works is that the builder starts with no directive definitions, and developers can add definitions for built-in directives if they want. at build-time
buildImplcallsensureBuiltInDirectiveswhich will add definitions for built-in directives that have not already been added by developers.i'm suggesting here the same behavior for
clearDirectives: it resets the state to what you would've gotten with a fresh builder (ie, no directives), and again we rely on the build process to ensure that all built-in directives are defined.but since we're talking about this behavior, i don't really like it, because (again as i understand it) the developer can add a definition of
skipfor example that doesn't have anifargument, which would cause chaos. so what we could do here is to always initialize the directives list of a builder to have our pre-defined built-in directive definitions -- this is what you'd get with a fresh builder, and this is what you'd get withclearDirectives-- and we throw an error if you attempt to redefine a built-in directive. alternatively,ensureBuiltinDirectivescould check that the def's provided for built-ins conform to the spec.in any event, i think that
clearDirectivesshould reset the directive-definitions state of aBuilderto the state you get from a fresh builder, no matter what we think that state should be...Re-add GraphQLSchema.Builder.clearDirectives()