grats icon indicating copy to clipboard operation
grats copied to clipboard

Support for directives

Open louy opened this issue 11 months ago • 42 comments

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?

louy avatar Jan 06 '25 16:01 louy

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.

captbaritone avatar Jan 06 '25 20:01 captbaritone

For anyone interested, I have just hacked a solution for now. This also serves as an example of directives we have:

import { MapperKind, mapSchema } from '@graphql-tools/utils';
import { GraphQLSchema } from 'graphql';
import type { Int } from 'grats';

type Directive = {
  name: 'complexity' | 'sensitive';
  args: Record<string, unknown>;
};

/**
 * Directive controlling query cost analytics
 */
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 {
  return {
    name: 'complexity',
    args: {
      value,
      multipliers,
      multiplyOnlyNestedFields,
      defaultMultiplier,
    },
  };
}

/** Marks a field as sensitive so it gets redacted in logs */
export function sensitive(
  /**An optional reason why the field is marked as sensitive*/
  reason?: string,
): Directive {
  return {
    name: 'sensitive',
    args: { reason },
  };
}

const directivesList: (
  | {
      directive: Directive;
      kind: 'FIELD';
      parent: string;
      name: string;
    }
  | {
      directive: Directive;
      kind: 'ARGUMENT';
      parent: string;
      field: string;
      argument: string;
    }
  | {
      directive: Directive;
      kind: 'TYPE';
      name: string;
    }
)[] = [];

export function addDirective(
  directive: Directive,
  kind: 'FIELD',
  parent: string,
  name: string,
): void;
export function addDirective(
  directive: Directive,
  kind: 'ARGUMENT',
  parent: string,
  field: string,
  argument: string,
): void;
export function addDirective(
  directive: Directive,
  kind: 'TYPE',
  name: string,
): void;
export function addDirective(
  directive: Directive,
  kind: 'TYPE' | 'FIELD' | 'ARGUMENT',
  arg1: string,
  arg2?: string,
  arg3?: string,
) {
  switch (kind) {
    case 'FIELD':
      directivesList.push({
        directive,
        kind,
        parent: arg1!,
        name: arg2!,
      });
      break;
    case 'ARGUMENT':
      directivesList.push({
        directive,
        kind,
        parent: arg1!,
        field: arg2!,
        argument: arg3!,
      });
      break;
    case 'TYPE':
      directivesList.push({
        directive,
        kind,
        name: arg1!,
      });
      break;
  }
}

export function applyDirectives(schema: GraphQLSchema) {
  const cloned = directivesList.slice();
  const mapped = mapSchema(schema, {
    [MapperKind.TYPE]: (type) => {
      const applicable = cloned.filter(
        (directive) =>
          directive.kind === 'TYPE' && directive.name === type.name,
      );

      if (applicable.length === 0) {
        return type;
      }

      type.extensions = type.extensions ?? {};

      const extensions: any = type.extensions;
      extensions.directives = extensions.directives ?? {};

      for (const item of applicable) {
        extensions.directives[item.directive.name] = item.directive.args;
        cloned.splice(cloned.indexOf(item), 1);
      }
    },

    // FIELD
    [MapperKind.FIELD](fieldConfig, fieldName, typeName) {
      const applicable = cloned.filter(
        (directive) =>
          (directive.kind === 'FIELD' &&
            directive.parent === typeName &&
            directive.name === fieldName) ||
          (directive.kind === 'ARGUMENT' &&
            directive.parent === typeName &&
            directive.field === fieldName),
      );

      if (applicable.length === 0) {
        return fieldConfig;
      }

      for (const item of applicable) {
        if (item.kind === 'FIELD') {
          fieldConfig.extensions = fieldConfig.extensions ?? {};
          const extensions: any = fieldConfig.extensions;
          extensions.directives = extensions.directives ?? {};

          extensions.directives[item.directive.name] = item.directive.args;
          cloned.splice(cloned.indexOf(item), 1);
        } else if (item.kind === 'ARGUMENT' && 'args' in fieldConfig) {
          fieldConfig.args = fieldConfig.args ?? {};
          const arg = fieldConfig.args[item.argument];
          if (arg) {
            arg.extensions = arg.extensions ?? {};
            const extensions: any = arg.extensions;
            extensions.directives = extensions.directives ?? {};

            extensions.directives[item.directive.name] = item.directive.args;
            cloned.splice(cloned.indexOf(item), 1);
          }
        }
      }
    },
  });
  if (cloned.length)
    throw new Error(
      `Found unresolved directives\n${JSON.stringify(cloned, null, 2)}`,
    );
  return mapped;
}

Usage:

addDirective(complexity(50), 'FIELD', 'Mutation', 'createFcmToken');
addDirective(sensitive(), 'ARGUMENT', 'Mutation', 'createFcmToken', 'token');
/**
 * Register Firebase Cloud Messaging token
 * @gqlMutationField
 */
export async function createFcmToken(
  /** @sensitive */
  token: string,
  { fetchFreshaApi }: Context,
): Promise<FcmToken | null> {
  // ...
}

And later:

applyDirectives(getSchema())

louy avatar Jan 07 '25 09:01 louy

Basically what I'd like to see is 2 things:

  1. Ability to declare directives, potentially from functions, similar to the above two exmaples I have:
/**
 * 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
  1. Abilty to apply directives to the various things they apply to: fields, arguments, types, enums, etc This one is a lot tricker because grats doesn't use decorators. My hack was to introduce a function you have to call, but it is not type-safe and I prefer a safer solution (maybe you wrap resolver functions in directive apply)

louy avatar Jan 07 '25 09:01 louy

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)

louy avatar Jan 07 '25 09:01 louy

Thanks for the examples, helps clarify!

I think a reasonable starting place would be:

Define directives

You 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 @gqlDirective could be followed by either ON <...directiveLocations> in which case the type alias's name would be the directive name, or <directiveName> ON <...directiveLocations> to support aliasing the type name. Aliasing will probably be common since directives generally use snakeCase but types generally use PascalCase.

Using directives

How 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 @someDirective(someArg: "Hello") as a docblock tag, or if we'll end up needing to do our own docblock parsing.

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.

captbaritone avatar Jan 07 '25 18:01 captbaritone

I like this a lot. Yes.

The other problem with parsing however is multiline directive args

I was thinking an alternative might be

/**
 * @gqlField
 * @someDirective.shortArg String
 * @someDirective.longArg A very long
 * multi line string
 * @someDirective.booleanArg true
 * @someDirective.enumArg ENUM_VAL
 */
export class User {
  // ...
}

But then repeatable args become unclear, for that maybe you allow using only the directive name to count as the start of the invocation:

/**
 * @gqlField
 * @someDirective
 * @someDirective.shortArg String1
 * @someDirective
 * @someDirective.shortArg String2
 */
export class User {
  // ...
}

louy avatar Jan 13 '25 10:01 louy

If we can agree on an approach here then I wouldn't mind contributing with an implementation.

felamaslen avatar Jan 14 '25 14:01 felamaslen

I think ideally it could be:

/**
 * @gqlField
 * @someDirective(someArg: "Hello")
 */
export class User {
  // ...
}

But we'll need to see if TypeScript will parse @someDirective(someArg: "Hello") as a docblock tag, or if we'll end up needing to do our own docblock parsing.

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.

Why not @gqlDecorate <directiveName> <directiveArg>...

/**
 * @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 @gql-prefixed doc tags. @gqlDirective to define a directive, and @gqlDecorate to decorate a target with an existing directive. I'm not sure of the exact JSDoc syntax supported by Typescript, but there's probably a way to make this work, using curly braces or other JSDoc syntax like:

/** @gqlDecorate `complexity(value: 20, motivation: "This arg requires extra backend queries")` */

Might be worth taking a look at what TSDoc supports.

leo-fresha avatar Jan 14 '25 18:01 leo-fresha

Thanks everyone for the feedback/ideas!

I looked into it a bit more and learned a few things:

  1. TypeScript will let me parse @someDirective(someArg: "foo") in a docblock as a docblock tag.
  2. Parsing such directives out of a docblock (by passing the text to the GraphQL-js parser and using special methods on the parser) is possible, but reporting parse errors as granular locations is not really viable due the case where the directive spans multiple lines but those lines are prefixed with * from the docblock. TS will strip those for us, so we can parse it fine, just not reliably point back to the correct source location
  3. If a directive does not have arguments, it becomes indistinguishable from a docblock tag. Since we don't know all valid docblocks at extraction time, this means we would need to retain all dobblock tags as possible directives and strip them later when we are full program aware.

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 @gqlDecorate as a name since it does not map to any well defined name/concept in GraphQL.

Maybe @gqlDirective followed by plain old SDL directives? I'll need to check if TS treats this as two tags or one.

/**
 * @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 @gqlDirective is used for adding directives...

As for @deprecated. I'd like to keep that using the existing @deprecated docblock tag since that is already a concept in TypeScript (it will render the function/method/etc as ~~struck through~~).

captbaritone avatar Jan 15 '25 01:01 captbaritone

Personally I would keep @gqlDirective to define a directive, and use another tag for "applying" them. @gqlDirective matches @gqlType, @gqlField etc used to define the other GraphQL concepts.

The GraphQL spec uses the term "annotate" consistently when talking about using directives, so what about @gqlAnnotate?

/**
 * @gqlAnnotate complexity(value:50)
 * or
 * @gqlAnnotate @complexity(value:50)
 */

A GraphQL schema describes directives which are used to annotate various parts of a GraphQL document

In this example, a directive is defined which can be used to annotate a field

Directives can also be used to annotate the type system definition language as well

In this example, the directive @example annotates field and argument definitions

Or, for a more verbose but unmistakable option, @gqlApplyDirective

leo-fresha avatar Jan 15 '25 08:01 leo-fresha

I've confirmed that TypeScript parser will treat

/**
 * @gqlAnnotate @complexity(value:50)
 */

As two separate tags, which is a bummer for us.

captbaritone avatar Jan 15 '25 20:01 captbaritone

I'm exploring the ability to use docblock tags as directives directly in this WIP PR: https://github.com/captbaritone/grats/pull/167

captbaritone avatar Jan 16 '25 05:01 captbaritone

Is there anything we can help with to move this forward?

louy avatar Jan 28 '25 11:01 louy

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.

captbaritone avatar Jan 29 '25 19:01 captbaritone

Okay, I made quite a bit of progress today (you can see that progress in this draft PR: https://github.com/captbaritone/grats/pull/167)

Here are a few things I'm stuck on, and could use help thinking of solutions to:

Accessing schema directives at runtime

The graphql-js GraphQLSchema object does not actually preserve schema directives. If your schema is generated from parsing an SDL file, you can dig into the (optional) AST property it exposes, but the GraphQLSchema object Grats generates typegen for does not support a way to specify that, say, a field has a directive on it. This makes it hard to define a proof of concept for how you would actually use these directives.

Syntax for applying directives

My 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 approach proposed above by @leo-fresha:

/**
 * @gqlAnnotate complexity(value:50)
 */

captbaritone avatar Jan 31 '25 23:01 captbaritone

Spent some time implementing @gqlAnnotate and realized that we'll need to find some consistency between how we support directives generically and how we support some built in directives. Currently there are a few which we currently support:

  • @deprecated Some reason
  • @specifiedBy https://example.com
  • @oneOf

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 @deprecated which currently has an expected format in TypeScript and I like the fact that we transfer the TypeScript semantics to GraphQL.

Maybe a nice middle ground would be @deprecated which is the only one with shared TypeScript and GraphQL semantics is the only one with special syntax, and all others revert back to the generic syntax.

captbaritone avatar Feb 01 '25 22:02 captbaritone

@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.

captbaritone avatar Feb 01 '25 22:02 captbaritone

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 @gqlAnnotate and via native syntax

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. The other issue is I worry devs don't add directives as often because they're really hard to write now.

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)

louy avatar Feb 06 '25 15:02 louy

I personally don't see why @specifiedBy and @oneOf should be supported as jsdoc directives

louy avatar Feb 06 '25 15:02 louy

It would also mean that all grats decorators are prefixed with @gql (except for ones that are native to jsdoc, i.e. @deprecated)

louy avatar Feb 06 '25 15:02 louy

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 avatar Feb 09 '25 22:02 captbaritone

@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
 */

leo-fresha avatar Feb 11 '25 14:02 leo-fresha

Oops. The second one is left over from an earlier iteration. I'll update

captbaritone avatar Feb 11 '25 14:02 captbaritone

The proposal looks good!

louy avatar Feb 19 '25 10:02 louy

As I worked through the implementation, I ended up pivoting back to this syntax:

/**
 * @gqlDirective on FIELD_DEFINITION
 */

I just merged https://github.com/captbaritone/grats/pull/167 which supports defining and adding directives with all the documentation:

  • https://grats.capt.dev/docs/docblock-tags/directive-definitions/
  • https://grats.capt.dev/docs/docblock-tags/directive-annotations/

[email protected] has these changes. Would you be able to try that out and give feedback before I ship a formal release?

captbaritone avatar Feb 20 '25 03:02 captbaritone

I've done a quick test and it works great

@felamaslen will be testing this against our (entire?) codebase

louy avatar Feb 21 '25 10:02 louy

@captbaritone thanks for sorting this out. It seems to work, except I now have the following issue:

Directive "@deprecated" may not be used on ENUM.

~Looks like a regression in application of @deprecated directive to enums?~

Oops, this actually looks like a mistake on our part. Enums cannot be deprecated

felamaslen avatar Feb 21 '25 10:02 felamaslen

Having done some more testing, this works really well 👍

There is a slight issue which is that the schema AST returned by getSchema does not contain the directive nodes on applied directives. This means the consumer is responsible for splicing these in, by parsing the extensions.grats object.

What do you think about potentially exposing the directives in the AST directly, so consumers do not have to be aware of this?

felamaslen avatar Feb 21 '25 12:02 felamaslen

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
    }
  )
}

felamaslen avatar Feb 21 '25 12:02 felamaslen

Enums cannot be deprecated

I'll add a note to the changelog clarifying that the new directive validation may uncover previously invalid deprecated directives.

captbaritone avatar Feb 21 '25 16:02 captbaritone