Skip to content
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

Make directives future-proof #571

Open
pavelnikolov opened this issue Feb 14, 2023 · 4 comments
Open

Make directives future-proof #571

pavelnikolov opened this issue Feb 14, 2023 · 4 comments
Milestone

Comments

@pavelnikolov
Copy link
Member

Currently the library only supports directives on FIELD_DEFINITION. When/if we add support for more directive locations, the users who already use the current directive API would face breaking changes. We need to make sure that we can add more directive visitor types and/or directive locations in future without introducing breaking changes.

@pavelnikolov
Copy link
Member Author

pavelnikolov commented Feb 23, 2023

Potentially we could add another method in the Directive interface:

type Directive interface {
    AllowLocation(l string) bool
    ImplementsDirective() string 
}

In future we might have a general directive on arbitrary locations, which would implement the following interface:

// ASTVisitor is a general directive visitor interface that could be invoked on various locations both
// at schema parse time and during requests.
type ASTVisitor interface {
    // Visit is invoked for every occurrence of the directive in the SDL schema or in the executable
    // definition document (i.e. request). It accepts the parsed schema and the target object it is
    // applied to which is a type from the [ast] package.
    Visit(ctx context.Context, s *ast.Schema, t interface{})
}

@pavelnikolov
Copy link
Member Author

pavelnikolov commented Feb 23, 2023

The above can even be split into two:

type ASTSchemaVisitor interface {
    Visit(ctx context.Context, s *ast.Schema, t interface{})
}

and

type ASTVisitor interface {
    Visit(ctx context.Context, s *ast.Schema, doc *ast.ExecutableDefinition, t interface{})
}

to differentiate between schema and request directives.
I'm not going to implement these now. I am just planning ahead and would like to add the AllowLocation(l string) bool method to the directive interface to prevent directives from stepping on each other's toes in future. This is a safety net that directives are only going to be executed in the correct location and are going to be ignored everywhere else.

cc @dackroyd

@pavelnikolov pavelnikolov modified the milestones: v1.6.0, v1.7.0 Mar 2, 2023
@ssko1
Copy link

ssko1 commented Apr 21, 2023

It would be awesome for directives applied to objects and input objects to modify the behavior of each field resolver in that object. Now that we have resolver interceptors and the resolver interface, I see this as being quite possible to achieve.

@dackroyd
Copy link
Contributor

#609 includes adding AllowLocation(l string) bool to the Directive interface, and combines directive based validation for fields and for field arguments into the same Validator interface. Renaming or otherwise implementing as ASTVisitor and ASTSchemaVisitor has been left for now, and will need to be explored before directives support is considered "stable"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants