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

getting directives from extensions does not follow gatesby convention as documented #2534

Closed
hayes opened this issue Feb 1, 2021 · 3 comments
Labels
waiting-for-release Fixed/resolved, and waiting for the next stable release

Comments

@hayes
Copy link

hayes commented Feb 1, 2021

In a few places throughout the docs, it is indicated that directives can be read from extensions following conventions used by gatsby, layed out in this comment: graphql/graphql-js#1343.

This comment suggests a specific format for how directives should be formatted in extensions. It looks like the format actually expected by the SchemaDirectiveVisitor (and the return type of getDirectives) is different than this format.

The gatsby format suggests directives be an array like:

{ 
   directives: [
    {
      name: 'directiveName'
      args: directiveArgs
    }
  ]
}

but instead the expected format is:

{ 
   directives: {
     directiveName: [directiveArgs]
     // or
     directiveName: directiveArgs
  }
}

This format loses information about the order of directives.

eg. there is no way to tell the difference between:

type Query @A @B @A {
  test: String
}

and

type Query @A @A @B {
  test: String
}

Suggested solutions in order of my own preference:

  1. Update directive parsing so that order is properly maintained when extensions.directives format matches gatsby convention, continue to support current format.
  2. Update getDirectives to handle gatsby format, and just convert into the current format (loses some order information, but allows code-first schema generators to generate extensions in a way that maintains order for compatibility with other tools).
  3. Don't support gatsby format, but document the expected format more clearly.
@yaacovCR
Copy link
Collaborator

yaacovCR commented Feb 1, 2021

Thanks for raising this issue. You are completely correct that we are not following the gatsby convention and that the ability to order repeatable directives around other directives is lost. I agree with you that option 1 seems best way to address. A pr would be very welcome.

@hayes
Copy link
Author

hayes commented Feb 1, 2021

Based on your careful wording above, I assume you are already aware of this, but for context of any other readers: This issue is not as bad as it might appear, and only affects repeatable directives. The current implementation can maintain order of non-repeating directives due to js respecting insertion order when iterating.

I don't know when I'll have time, but I could probably take a stab at a PR.

I can see a few different ways to implement this:

  1. update getDirectives and any place it is called to normalize to the gatsby convention (breaking change)
  2. update getDirectives with a flag that when enabled converts to gatsby format, and use that flag in places like the schema visitor (probably not breaking)
  3. Add a new method that gets directives in gatsby format (non-breaking)
  4. Just update the return type of getDirectives to be either the current format or the gatsby format, and let consumers figure it out.

Having getDirective (or a new similar method) that always returns a consistent format (gatsby format) would be my preferred experience as a user. Having to always account for multiple formats in any usage would be a bit of a pain. This is also potentially the hardest sell, because its a breaking change, and any existing consumers of that method would need to update.

Any thoughts on the best way to implement this?

yaacovCR added a commit that referenced this issue Jul 12, 2021
see #2534

BREAKING CHANGE: getDirectives now always return an array of DirectiveAnnotation objects

New function getDirective returns an array of args records for each use of the directive, Note: this is true even when the directive is non-repeatable.
@ardatan ardatan added the waiting-for-release Fixed/resolved, and waiting for the next stable release label Jul 12, 2021
@ardatan
Copy link
Owner

ardatan commented Jul 28, 2021

Available in the latest version!

@ardatan ardatan closed this as completed Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-release Fixed/resolved, and waiting for the next stable release
Projects
None yet
Development

No branches or pull requests

3 participants