-
Notifications
You must be signed in to change notification settings - Fork 17
Support for directives #166
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
Comments
Grats does not currently support adding or defining arbitrary schema directives, but it seems like a reasonable thing to support. Could you tell me more about the specific directives you you use that you'd like to support? Would help inform how they should be supported. |
For anyone interested, I have just hacked a solution for now. This also serves as an example of directives we have:
Usage:
And later:
|
Basically what I'd like to see is 2 things:
/**
* Directive controlling query cost analytics
* @gqlDirective on FIELD_DEFINITION
*/
export function complexity(
/** The complexity value for the field - works as described in (graphql-query-complexity)[https://github.com/slicknode/graphql-query-complexity/blob/master/src/estimators/directive/README.md]
*
*/
value: Int,
/** Information how field arguments influence cost of returned list - works as described in (graphql-query-complexity)[https://github.com/slicknode/graphql-query-complexity/blob/master/src/estimators/directive/README.md]
*
*/
multipliers?: string[],
/** Cost of field assign to directive doesn't depend on multiplier but cost of nested fields yes
*
*/
multiplyOnlyNestedFields?: boolean,
/** In situation where we cannot define multiplier depending on arguments we use defaultValue to multiple
*
*/
defaultMultiplier?: Int,
): Directive {} Generates "Directive controlling query cost analytics"
directive @complexity(
"The complexity value for the field - works as described in (graphql-query-complexity)[https://github.com/slicknode/graphql-query-complexity/blob/master/src/estimators/directive/README.md]"
value: Int!
"Information how field arguments influence cost of returned list - works as described in (graphql-query-complexity)[https://github.com/slicknode/graphql-query-complexity/blob/master/src/estimators/directive/README.md]"
multipliers: [String!]
"Cost of field assign to directive doesn't depend on multiplier but cost of nested fields yes"
multiplyOnlyNestedFields: Boolean
"In situation where we cannot define multiplier depending on arguments we use defaultValue to multiple"
defaultMultiplier: Int
) on FIELD_DEFINITION
|
Directives don't really have logic like resolvers, so maybe a function doesn't make sense. Usually directives are used either to apply schema transformations (using mapSchema) or to be read later in resolvers/middleware (like the sensitive directive above, used to mask values in the logging middleware) |
Thanks for the examples, helps clarify! I think a reasonable starting place would be: Define directivesYou can define a directive on a type alias of an object literal type describing the directive's arguments: /**
* Directive controlling query cost analytics
* @gqlDirective complexity on FIELD_DEFINITION
*/
type Complexity = {
/** The complexity value for the field - works as described in (graphql-query-complexity)[https://github.com/slicknode/graphql-query-complexity/blob/master/src/estimators/directive/README.md]
*
*/
value: Int,
/** Information how field arguments influence cost of returned list - works as described in (graphql-query-complexity)[https://github.com/slicknode/graphql-query-complexity/blob/master/src/estimators/directive/README.md]
*
*/
multipliers?: string[],
/** Cost of field assign to directive doesn't depend on multiplier but cost of nested fields yes
*
*/
multiplyOnlyNestedFields?: boolean,
/** In situation where we cannot define multiplier depending on arguments we use defaultValue to multiple
*
*/
defaultMultiplier?: Int,
}; The Using directivesHow do you imagine the syntax for passing arguments to directives would look? I think ideally it could be: /**
* @gqlField
* @someDirective(someArg: "Hello")
*/
export class User {
// ...
} But we'll need to see if TypeScript will parse One thing that will be tricky is knowing which docblock tags should be interpreted as directives and which are unrelated to Grats. Elsewhere in Grats we handle this by parsing everything as if it belongs to Grats, and then in a later phase (when we know which names exist) we discard any which we learn are not related to Grats. |
I like this a lot. Yes. The other problem with parsing however is multiline directive args I was thinking an alternative might be
But then repeatable args become unclear, for that maybe you allow using only the directive name to count as the start of the invocation:
|
If we can agree on an approach here then I wouldn't mind contributing with an implementation. |
Why not /**
* @gqlType
* @gqlDecorate cacheControl scope:PRIVATE maxAge:100
*/
type Wallet = {
/**
* @gqlField
* @gqlDecorate complexity value:50
*/
balance: Int,
/** @gqlField */
paymentMethods(
/**
* @gqlArg
* @gqlDecorate deprecated
*/
includeExpired: boolean | null
): [PaymentMethod]
} It keeps with using /** @gqlDecorate `complexity(value: 20, motivation: "This arg requires extra backend queries")` */ Might be worth taking a look at what TSDoc supports. |
Thanks everyone for the feedback/ideas! I looked into it a bit more and learned a few things:
I think this points to something like what @leo-fresha is describing, where there's a tag that denotes "directive here" and the tag value is the directive. That said, I'm not crazy about introducing special syntax here which deviates from both GraphQL and typescript: directives without an @, required quotes, a space after directive name... I'm also not crazy about Maybe /**
* @gqlType
* @gqlDirective @cacheControl(scope:PRIVATE maxAge:100)
*/
type Wallet = {
/**
* @gqlField
* @gqlDirective @complexity(value:50)
*/
balance: Int, That still leaves open: How do we define directives if As for |
Personally I would keep The GraphQL spec uses the term "annotate" consistently when talking about using directives, so what about /**
* @gqlAnnotate complexity(value:50)
* or
* @gqlAnnotate @complexity(value:50)
*/
Or, for a more verbose but unmistakable option, |
I've confirmed that TypeScript parser will treat /**
* @gqlAnnotate @complexity(value:50)
*/ As two separate tags, which is a bummer for us. |
I'm exploring the ability to use docblock tags as directives directly in this WIP PR: #167 |
Is there anything we can help with to move this forward? |
I think I have a path forward in the above draft PR. I need to add support for directive validation (surprisingly it's not provided by regular schema validation) and I also want to revisit the API design for applying directives (just using docblock tags might pose an unacceptable tradeoff due to the risk of namespace collisions) I suspect I'll have time to make progress on Friday. |
Okay, I made quite a bit of progress today (you can see that progress in this draft PR: #167)
Here are a few things I'm stuck on, and could use help thinking of solutions to: Accessing schema directives at runtimeThe Syntax for applying directivesMy current approach is just to treat docblock tags that match directive names as directives and parse them as SDL. So: /**
* @gqlQueryField
* @cost(credits: 10)
*/
export function greeting(): string {
return "hello";
} This is intuitive and simple (no new syntax) but it means if there's ever a namespace collision between docblock tag names you want to use and directive names you want to use, you might get stuck. I'm going take a stab at the /**
* @gqlAnnotate complexity(value:50)
*/ |
Spent some time implementing
I think that's all of them? Maybe when we add support for generic directives we should drop special handling for these and instead ask users to write them in whatever generic way we settle on? I think the biggest tension here is Maybe a nice middle ground would be |
@louy Just curious what your specific urgency is? Are you need a resolution here before you can decide which GraphQL solution you are going to use? Or is it that it's blocking a specific use case. If it's the latter, maybe we can come up with some workaround to unblock you in the meantime. |
I think deprecated being special is fair, but also keep in mind people can potentially re-declare deprecated and add more arguments to it, so would be nice if it was both supported via Re: the urgency Grats has been great overall. We've adopted it over codegen as it is tons better. However, it has made directives (which we use very heavily) very awkward. We have custom linting rules than enforce certain directives that we're unable to migrate to grats because our hacky solution (the one I documented in the first few comments) is really ugly. As I said before, we're happy to support with this effort (contribute time) as it's not fair to put expectations on open-source maintainers (and you don't owe us anything) |
I personally don't see why |
It would also mean that all grats decorators are prefixed with |
Okay. I think I've figured out what syntax is going to be best for Grats. To validate it for myself I've written a first draft of two new doc pages trying to fully explain how to both define and use directives. I'd love a pass of feedback on this if you have time. |
@captbaritone those drafts look good to me. One thing that is confusing me is that here https://gist.github.com/captbaritone/6f8369241989bf1afa518acced5287a7 there are examples with two different syntaxes to define a directive, and I am not clear on which one is the correct one (or are both allowed?) /**
* @gqlDirective
* @gqlOn FIELD_DEFINITION
*/ /**
* @gqlDirective on FIELD_DEFINITION
*/ |
Oops. The second one is left over from an earlier iteration. I'll update |
The proposal looks good! |
As I worked through the implementation, I ended up pivoting back to this syntax:
I just merged #167 which supports defining and adding directives with all the documentation:
|
I've done a quick test and it works great @felamaslen will be testing this against our (entire?) codebase |
@captbaritone thanks for sorting this out. It seems to work, except I now have the following issue:
Oops, this actually looks like a mistake on our part. Enums cannot be deprecated |
Having done some more testing, this works really well 👍 There is a slight issue which is that the schema AST returned by What do you think about potentially exposing the directives in the AST directly, so consumers do not have to be aware of this? |
FYI this is how I am currently applying the directives based on the extensions: type GratsExtensions = {
grats?: {
directives?: { name: string; args?: Record<string, any> }[];
};
};
function Directive(
name: string,
args: ConstDirectiveNode['arguments'],
): ConstDirectiveNode {
return {
kind: Kind.DIRECTIVE,
name: { kind: Kind.NAME, value: name },
arguments: args,
};
}
function Argument(
name: string,
value: ConstArgumentNode['value'],
): ConstArgumentNode {
return {
kind: Kind.ARGUMENT,
name: { kind: Kind.NAME, value: name },
value,
};
}
function extractDirective(
schema: GraphQLSchema,
directiveName: string,
args: Record<string, any> | undefined,
): ConstDirectiveNode {
const directive = schema.getDirective(directiveName);
assert(directive, `Directive with name ${directiveName} not found in schema`);
return Directive(
directive.name,
Object.entries(args ?? {}).map(([argName, argValue]) => {
const argDefinition = directive.args.find((arg) => arg.name === argName);
assert(
argDefinition,
`Directive arg on ${directiveName} with name ${argName} not found`,
);
const value = ((): ConstArgumentNode['value'] => {
switch (argDefinition.type) {
case GraphQLInt:
return { kind: Kind.INT, value: argValue };
// TODO(FM): support other primitive directive arg types, e.g. String
default: {
const customType = schema.getType(argDefinition.type.toString());
if (customType instanceof GraphQLEnumType) {
return {
kind: Kind.ENUM,
value: argValue,
};
} else {
// TODO(FM): support other custom directive arg types, e.g. ObjectValueNode
throw new Error(
`Unknown type ${argDefinition.type} on directive arg ${directiveName}.${argDefinition.name}`,
);
}
}
}
})();
return Argument(argDefinition.name, value);
}),
);
}
export function applyDirectives(schema: GraphQLSchema) {
return mapSchema(
schema,
{
[MapperKind.FIELD](fieldConfig, fieldName, typeName) {
const gratsDirectives =
(fieldConfig.extensions as GratsExtensions).grats?.directives ?? [];
for (const directive of gratsDirectives) {
const directives = fieldConfig.astNode!.directives?.slice() ?? [];
(fieldConfig.astNode!.directives as any) = directives;
const directiveNode = extractDirective(
schema,
directive.name,
directive.args,
);
directives.push(directiveNode);
}
},
// similar for MapperKind.TYPE
}
)
} |
I'll add a note to the changelog clarifying that the new directive validation may uncover previously invalid deprecated directives. |
Hmm, I'm a little perplexed. Why do you need the AST of the directive definitions? Maybe you could share what is is that you are doing with the value you get from calling |
In our case, we need the schema AST with directives applied, in order to print the schema into a There are probably other use cases though for having an AST fully representative of the generated schema. However, the It would be helpful IMHO to expose a util like |
FYI, The shape of The important use-case for directives is transforms. It would be great if switching from
Alternatively, and especially during the migration/incremental adoption of grats, all our transforms would have to be rewritten to call Thoughts? @captbaritone Edit: We now use |
Is there a reason the |
@louy Thanks for the great breakdown! Very cool to see that Could you tell me more about what transformations are in play here? I'm wondering if you actually need to be running them on the Knowing more about what these transforms are actually being used for would help me understand the viability of that approach. |
In our case, we merge the schema with some legacy type defs prior to generating the .graphql file. Regarding transforms, for a given directive we might define a function, mapping |
FYI, we have started implementing this, using the following mapping to convert the import { MapperKind, mapSchema } from '@graphql-tools/utils';
type GratsExtensions = {
grats?: {
directives?: { name: string; args?: Record<string, any> }[];
};
};
export function applyDirectives(schema: GraphQLSchema): GraphQLSchema {
const mapped = mapSchema(
schema,
{
[MapperKind.TYPE]: (type) => {
const gratsDirectives = (type.extensions as GratsExtensions).grats
?.directives;
if (gratsDirectives) {
Object.assign(type.extensions as any, {
directives: Object.fromEntries([
...Object.entries(type.extensions?.directives ?? {}),
...gratsDirectives.map((directive) => [
directive.name,
directive.args ?? {},
]),
]),
});
}
return type;
},
[MapperKind.FIELD](fieldConfig, _fieldName, _typeName) {
const gratsDirectives = (fieldConfig.extensions as GratsExtensions)
.grats?.directives;
if (gratsDirectives) {
Object.assign(fieldConfig.extensions as any, {
directives: Object.fromEntries([
...Object.entries(fieldConfig.extensions?.directives ?? {}),
...gratsDirectives.map((directive) => [
directive.name,
directive.args ?? {},
]),
]),
});
}
return fieldConfig;
},
[MapperKind.ARGUMENT](argumentConfig, _fieldName, _typeName) {
const gratsDirectives = (argumentConfig.extensions as GratsExtensions)
.grats?.directives;
if (gratsDirectives) {
Object.assign(argumentConfig.extensions as any, {
directives: Object.fromEntries([
...Object.entries(argumentConfig.extensions?.directives ?? {}),
...gratsDirectives.map((directive) => [
directive.name,
directive.args ?? {},
]),
]),
});
}
return argumentConfig;
},
},
);
} |
Thanks! This is very helpful. Is the reason you want to use |
Yup it's because there are grats and non-grats schemas (codebase is partially migrated). |
Btw I've been looking at other codebases to see how they consume directives in their transforms, and some (e.g. So my proposal above (of writing to But it would be nice if some libraries would work OOTB, as opposed to none that would work with grats schema today without tinkering with config (that is often not even exposed). Examples of transforms that use
Examples of transforms that use |
@captbaritone I like this I've hit a problem definining /**
* @gqlDirective on FRAGMENT_SPREAD | INLINE_FRAGMENT
*/
function defer({
label,
if = true, // Syntax error (reserved word)
}: {
label: string;
if?: boolean | null;
}): void {} /**
* @gqlDirective on FRAGMENT_SPREAD | INLINE_FRAGMENT
*/
function defer({
label,
'if': _ = true, // anonymous alias
}: {
label: string;
if?: boolean | null;
}): void {} parses, but the output is missing the default value directive @defer(label: String!, if: Boolean) on FRAGMENT_SPREAD | INLINE_FRAGMENT should be directive @defer(label: String!, if: Boolean = true) on FRAGMENT_SPREAD | INLINE_FRAGMENT |
@tomgasson Good catch: https://github.com/captbaritone/grats/pull/185/files Update: Shipped in |
Hi there,
Awesome tool. We're strongly considering using it. Quick question on directives because I can't find anything in the docs:
How do you declare directives? How do you annotate different fields/types with directives?
The text was updated successfully, but these errors were encountered: