feat: Add Schema Directives to SDL #952

tgriesser · 2021-07-21T13:45:28Z

Helps with: #148

WIP: Still needs docs / additional tests / cleanup of a bit of the builder layer / fixing types

Adds a directives property to all definition props which takes an array of directives. Adds them to an astNode which are then printed via a custom schema printer.

Definition:

const someDirective = directive({
  name: 'SomeDirective',
  locations: ['OBJECT'],
  args: {
    bool: nonNull(booleanArg())
  }
})

Use:

objectType({
  name: 'SomeType',
  directives: [someDirective({bool: true})]
})

or

import { addDirective } from 'nexus'

objectType({
  name: 'SomeType',
  directives: [addDirective('SomeDirective', {bool: true})]
})

Prints:

type SomeType @SomeDirective(bool: true) {
  # ... fields
}

See changes in the kitchenSink/__app.ts for more example uses.

Sytten · 2021-07-21T14:05:51Z

Dont close #148, still needs a plugin for it to make is nice for people to use (should be trivial once this is merged)

Sytten · 2021-07-21T22:29:23Z

src/definitions/directive.ts

+  directiveUse: NexusDirectiveUse,
+  directiveDef: GraphQLDirective
+): ReadonlyArray<ArgumentNode> {
+  return mapObj(directiveUse.args ?? {}, (val, key) => {


I believe args in NexusDirectiveUse is an array since it uses MaybeArgsFor which maps to an array, I might be missing something though.
It is typed as any in my editor currently.

Hmm... Need to check on that

Sytten · 2021-07-21T22:35:27Z

src/definitions/args.ts

+   * @example
+   *   directives: [useDirective('ExampleDirective', { arg: true })]
+   */
+  directives?: Directives


General comment: Do you think it would be possible to type the Directives here to make sure the location matches the type? Currently it would fail at build time so I guess it's not too bad.

Yeah I had initially tried this api as:

directives(t) { t.MyDirective(args) t.OtherDirective(args) }

Where the members were dynamically based on directive declarations & where they can be positioned. It generally worked, but the types were pretty gnarly & seemed to be a lot of complexity for something that could just be a simple runtime error.

Sytten · 2021-07-21T22:43:54Z

src/definitions/directive.ts

+export function directive<DirectiveName extends string>(
+  config: NexusDirectiveConfig<DirectiveName>
+): NexusDirectiveDef<DirectiveName> {
+  assertValidName(config.name)


We also need to assert the name of the arguments too, is that done somewhere else?

Actually recently found that we don't do this for field names, but we should 😬

Sytten · 2021-07-21T22:52:58Z

src/definitions/directive.ts

+   *     locations: ['FIELD_DEFINITION'],
+   *   })
+   */
+  args?: ArgsRecord


General question since I am not sure, are we dealing with loops (direct or indirect) inside the directive?
https://spec.graphql.org/June2018/#sec-Type-System.Directives (see the validation at the end)
Say:

export const TestDirective = directive({ name: 'TestDirective', args: { someArg: arg({ type: 'TestType'}) }, locations: ['FIELD_DEFINITION'], }) export const TestType = objectType({ name: 'TestType', definition(t) { t.string('myField', { directives: [TestDirective({ myField: "bob" })]}) } })

No, good catch - need to do that

Sytten · 2021-07-21T23:09:40Z

src/printSchemaWithDirectives.ts

+
+export function printSchemaWithDirectives(schema: GraphQLSchema): string {
+  return printFilteredSchemaWithDirectives(schema, (n) => !isSpecifiedDirective(n), isDefinedType)
+}


I don't think we need a custom printer, all graphql versions that nexus support have a schema printer that does print the directives:
https://github.com/graphql/graphql-js/blob/v14.5.8/src/utilities/schemaPrinter.js
https://github.com/graphql/graphql-js/blob/v15.5.1/src/utilities/printSchema.js
And they should be exported by graphql/utilities.

I believe it only prints the directives declarations, not the directive uses on the SDL types/fields

Good point, bit annoying that we have to copy paste/maintain all that code. We should reference the commit it was taken from and maybe the diff vs the original to reduce upgrade burden.

Yeah, I'm not too worried about it in the sense that since it's just printing code the paths are pretty simple and easy to validate correctness of (not a lot of business logic that you'd otherwise be worried about with copy/paste).

Sytten · 2021-07-21T23:12:22Z

src/makeSchema.ts

  )
  assertNoMissingTypes(schema, missingTypes)
  runAbstractTypeRuntimeChecks(schema, finalConfig.features)
  return { schema, schemaTypes, tsTypes }
 }
+
+/** Builds the schema, we may return more than just the schema from this one day. */
+export function makeSchemaInternal(config: SchemaConfig) {


Do we want to export that anymore?

Yeah I think I'm using it in the tests, I don't believe it's publicly exported on the module

Sytten · 2021-07-21T23:16:46Z

src/makeSchema.ts

+      nexus: schemaExtension,
+    },
+    directives: [...specifiedDirectives, ...Object.values(customDirectives)],
+    ...schemaDirectives,


I have a TS error in editor for that line Property 'operationTypes' is missing in type 'ScalarTypeDefinitionNode' but required in type 'SchemaDefinitionNode'

I feel this is misleading for future changes, this returns an AST node. If some would need to override another property in the astNode they might override the astNode set by schemaDirectives.

Yeah agreed, meant to come back to this / mark this as a TODO

Sytten · 2021-07-21T23:22:52Z

src/builder.ts

@@ -869,6 +931,7 @@ export class SchemaBuilder {
        ...config.extensions,
        nexus: new NexusInputObjectTypeExtension(config),
      },
+      ...this.maybeAddDirectiveUses('INPUT_OBJECT', config.directives),


I said it in the makeSchema, but I want to drive the point, since this adds a whole astNode I feel that someone that might want to modify the AST node for another purpose will get blocked by this code or might override it entirely. I think we should at the very least wrap that in an array and a merge function or even better if only the directives field of the node was returned by maybeAddDirectiveUses (free and member functions) and the astNode was generated by another method.

I feel that someone that might want to modify the AST node for another purpose will get blocked by this code or might override it entirely

Yeah, fair points - I had considered that and was thinking of just crossing that bridge if/when it came to it, since astNode isn't part of the public api for any of the nexus builder fn's, I wasn't sure if that would actually happen in practice.

In theory no, we should never touch the AST node since it is supposed to the result of the parsing of the schema.
I still think it should be explicit in the code that we are converting the directives to an AST node. Something like:

this.createAstNode([this.maybeAddDirectiveUses])

I suspect that a future release of graphql will add backend directives in the spec, so it will just be easier to move to that then.

@tgriesser what would be the shortest path to unblock this pull request so it can go through?

@zingerj this is the blocker.

Going to punt on this for now - going to assume that if you need to modify the ASTNode you can use something else more specialized like graphql-tools

victoriris · 2021-08-27T15:39:23Z

Is there an estimated time for this new feature? 👀

* main: feat: ability to rename root types (#976) docs: Remove `.ts` extension from import statement (#974) fix: revert declareInputs default to false (#975) fix: printer imports, follow up from #967 (#970) fix: plugin inputFieldDefTypes (#919) fix: add globalHeaders to imports for global def file (#969) fix: correct imports for changes in #967 (#968) feat: ConfiguredTypegen for splitting global types & configuring input type emission (#967) Revert "fix: #921 wrong typegen for nullable values w/ default (#965)" (#966) fix: #921 wrong typegen for nullable values w/ default (#965)

tgriesser · 2021-09-02T13:54:23Z

Is there an estimated time for this new feature? 👀

Going to probably work on it a bit today/tomorrow.

sooxt98 · 2021-09-30T13:55:50Z

Does it work on inputObjectType?

export const InputType = inputObjectType({
  name: 'InputType',
  directives: // here!
  definition(t) {
    t.nonNull.string('key', {
      directives: // here!
    })
    t.int('answer')
  },
})

davidsoderberg · 2021-12-26T15:24:16Z

Any progress on this one?

yasaichi · 2021-12-27T14:23:41Z

What's a blocker of merging this PR? Failed checks?

victoriris · 2021-12-27T15:16:52Z

@davidsoderberg & @yasaichi just to let you know... I needed this for Apollo Federation support and ended up switching to type-graphql a month ago. Maybe it can help you too

davidsoderberg · 2021-12-30T21:04:27Z

I also ended up switching to type-graphql... :(

zingerj · 2022-02-07T23:32:52Z

Hey all, what's the latest on getting this merged? Looks like there's one failing test; is that it to get this across the line? Thanks for your work here!

YElyousfi · 2022-04-11T17:42:16Z

When can we expect this to get merged? Federation relies on directives.

meabed · 2022-04-30T05:22:28Z

This is great feature - thank you!!
Would it be ready to merge soon?

yasaichi · 2022-05-04T11:55:24Z

[FYI] To those who can't wait for this PR merged
You have the following alternative libraries supporting directives:

TypeGraphQL (mentioned above)
Pothos (formerly GiraphQL)

BahaaZidan · 2022-05-11T13:27:16Z

Please merge this!

* main: (33 commits) fix: incorrect logic in backward pagination (#1084) fix: update snapshots for change in #1083 chore: change facebook.github.io to relay.dev links (#1083) feat: allow specifying custom directives in makeSchema (#1065) Update comment for `shouldGenerateArtifacts` (#1057) feat: Run both formatTypegen and prettier formatter if given (#1042) feat: allow specifying custom directives in makeSchema (#1064) feat: Use ReadonlyArray in typings (#1041) chore: use dripip reusable workflow docs: Update npm badge v1.3.0 chore: add test confirming v16 schema compat (#1054) chore: update github workflows (#1053) feat: add GraphQL 16 support (#977) chore(docs): fix typos (#1005) chore(docs): fix typo 06-chapter-5-persisting-data-via-prisma.mdx (#1007) chore(docs): fix typo 04-why-nexus.mdx (#1008) chore(docs): fix typo 030-neuxs-framework-prisma-users.mdx (#1016) chore(docs): fix typo 05-chapter-4-testing-your-api.mdx (#1023) chore(docs): fix typo in 07-chapter-6-testing-with-prisma.mdx (#1024) ...

* main: chore: attempt to fix the dripip workflow, again chore: update the dripip workflow, bump lint-staged

tgriesser · 2022-05-15T21:13:50Z

This is merged, and is now released under nexus@1.4.0-next.7, no docs yet other than in the PR description.

No official plugin for federation, but the primitives are there - feel free to open a PR if you make progress on something generic and would like to add a plugin to the core.

Additionally, please give any feedback or let me know if you run into issues with it, will get this into an official 1.4.0 soon

meabed · 2022-06-11T06:18:16Z

@tgriesser - Thank you! any date we expecting this to be released?

cqhung1412 · 2024-03-19T09:16:42Z

@tgriesser - Please release this soon with the docs.

iamandrewluca · 2024-05-06T09:20:46Z

Hey folks! Thanks for working on this feature for Nexus!

Should there be any type of completion for the directive args when using it? 🤔

I've augmented it using this

declare module 'nexus/dist/core' {
  export interface NexusDirectiveDef {
    (arg: CacheHint): NexusDirectiveUse<"cacheControl">;
  }
}

PS: Does anyone know when a new minor release will be done with this feature?

Not related: I'm using this feature to implement cache control in Strapi. I started with using resolvers, but I think the directive approach is much better.

strapi/strapi#20254

tgriesser mentioned this pull request Jul 21, 2021

Support for operation directives #947

Closed

tgriesser requested review from Sytten and jasonkuhrt July 21, 2021 13:48

tgriesser added 2 commits July 21, 2021 12:20

feat: Add Directives / print SDL directives

d347793

fix: types for GraphQL 14

8524a96

tgriesser force-pushed the tgriesser/feat/sdl-directives branch from 62577bb to 8524a96 Compare July 21, 2021 16:20

Sytten requested changes Jul 21, 2021

View reviewed changes

Sytten mentioned this pull request Jul 26, 2021

How to check input integer range? #861

Open

redbar0n mentioned this pull request Oct 17, 2021

Feat: Autogenerated Typescript for OGM neo4j/graphql#528

Merged

tgriesser mentioned this pull request Mar 24, 2022

feat: allow specifying custom directives in makeSchema #1064

Merged

tgriesser added 6 commits May 15, 2022 12:46

rename useDirective -> addDirective

40fa55d

Fix tests

635c013

fix types

061279a

Merge branch 'main' into tgriesser/feat/sdl-directives

a2e82bf

* main: chore: attempt to fix the dripip workflow, again chore: update the dripip workflow, bump lint-staged

attempt to make things work with v15 & v16

4b625a1

tgriesser added 3 commits May 15, 2022 16:14

fixes

ef940ea

fix types, simplify the checking in core helpers

2981f0f

Remove unused import

91b83b8

tgriesser changed the title ~~feat: Add Directives / print SDL directives~~ feat: Add Schema Directives May 15, 2022

tgriesser changed the title ~~feat: Add Schema Directives~~ feat: Add Schema Directives to SDL May 15, 2022

tgriesser merged commit 9875e90 into main May 15, 2022

tgriesser deleted the tgriesser/feat/sdl-directives branch May 15, 2022 20:59

This was referenced May 16, 2022

fix: add missing as const on the RequestDirectiveLocation #1090

Merged

fix: remove hasSDLDirectives internal state #1091

Merged

feat: Add Schema Directives to SDL #952

feat: Add Schema Directives to SDL #952

Conversation

tgriesser commented Jul 21, 2021 • edited

Sytten commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgriesser Jul 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victoriris commented Aug 27, 2021

tgriesser commented Sep 2, 2021

sooxt98 commented Sep 30, 2021

davidsoderberg commented Dec 26, 2021

yasaichi commented Dec 27, 2021 • edited

victoriris commented Dec 27, 2021 • edited

davidsoderberg commented Dec 30, 2021

zingerj commented Feb 7, 2022

YElyousfi commented Apr 11, 2022

meabed commented Apr 30, 2022

yasaichi commented May 4, 2022 • edited

BahaaZidan commented May 11, 2022

tgriesser commented May 15, 2022

meabed commented Jun 11, 2022

cqhung1412 commented Mar 19, 2024

iamandrewluca commented May 6, 2024 • edited

tgriesser commented Jul 21, 2021 •

edited

tgriesser Jul 22, 2021 •

edited

yasaichi commented Dec 27, 2021 •

edited

victoriris commented Dec 27, 2021 •

edited

yasaichi commented May 4, 2022 •

edited

iamandrewluca commented May 6, 2024 •

edited