graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
19.97k stars 2k forks source link

Setting directives using ObjectType constructor (not IDL) #1343

Open MichalLytek opened 6 years ago

MichalLytek commented 6 years ago

I am reviving #1262 as I have an use case that is related to that discussion:

Right now directives are purely a feature of the GraphQL language and IDL, so a schema not created using IDL definitionally won't contain directives. When building a schema in code, you have the full power of the host programming environment, so directives shouldn't be necessary. Directives are a tool for supplying additional intent to be later interpreted by the host programming environment.

You can include any arbitrary data on a GraphQLObjectType that you like - it's just a JS object after all. Directives are a language-level feature, not a structural property of a type.

I agree that "directives are a tool for supplying additional intent" and "you have the full power of the host programming environment". But since the GraphQL ecosystem has grown, I think we should think about interoperability with 3rd-party libraries. The examples that I am thinking about is Apollo cache control and Prisma, which are using directives to provide some metadata to make awesome underlaying features work.

As graphql-js doesn't allow for registering directives using imperative way (ObjectType constructor), to be able to use Prisma, we are forced to use IDL to create our types. But this way might be not available when you are using the imperative way as you build a library on top of graphql-js, as well as when you would have to refactor your whole app by migrating to IDL to add apollo-cache integration.

So I think that now directives are a much more widespread feature and we should allow for interoperability with this 3rd-party tools.

MichalLytek commented 6 years ago

After 3 days, ping @leebyron as an author of the quotes and ping @IvanGoncharov as an author of #746 that have added support for directives ๐Ÿ˜ƒ

IvanGoncharov commented 6 years ago

@19majkel94 My position is the same as I wrote in #1262

I think this question is connected to the topic discussed on the last WG: Exposing schema metadata. So I think we should decide what we expose through introspection(directives or something else) and then add the same thing to GraphQL*Type classes.

nodkz commented 6 years ago

The new Apollo Server v2 introduce requirements for graphql-compose (and graphql-js) users to somehow provide directive via GraphQL*Type classes for providing an info for CDN cache. Without providing directives via GraphQL*Type we have no way to use Apollo Server v2.

Another use case was with graphql-cost-analysis which allows rejecting complex queries. It provides a comfortable way to add a complexity calculation logic without modifying resolvers.

It will be cool if Lee changes its position according to directives.

Repeat here a desired way for declaration directives from #1262:

const UserType = new GraphQLObjectType({
  name: 'User',
  fields: {
    name: {
      type: GraphQLString,
      description: '...',
      resolve: () => ...,
      directives: [
        { name: 'cost',  args: { useMultipliers: false, complexity: 2 } },
        { name: 'unique' },
      ],
      // or maybe simpler if graphql spec does not allow duplicated names
      directives: {
        cost: { useMultipliers: false, complexity: 2 },
        unique: true,
      },
    },
  },
});
nodkz commented 5 years ago

@IvanGoncharov @taion maybe you take care about this feature in the upcoming WG meetup 2018-09-20?

My English does not alow to me to do it myself ๐Ÿ˜Š

taion commented 5 years ago

The cost analysis case example is interesting, because in the context of using the programmatic API, I think it's suboptimal to set up costing via directives. Instead of the above, a better way to express the same would be to add a getCost(childCost, variables) callback to the field, then do something like:

myConnection: {
  type: FooConnection,
  getCost: (childCost, { first }) => first * childCost,
},
leebyron commented 5 years ago

Directives are purely a feature of the GraphQL language and are not part of type definitions - this is because different tools will use and interpret directives in different ways.

I assume the Apollo tools translate these GraphQL language directives to some other kind of metadata on the underlying schema and type objects - it would be best to use whatever API the apollo tools expect directly.

nodkz commented 5 years ago

@leebyron Now I understood what you mean. It can be explained in another way:

For the query:

query {
  authors {
    id
    name @uppercase
  }
}

needs to create such directive:

const UppercaseDirective = new GraphQLDirective({
  name: 'uppercase',
  description: 'Provides default value for input field.',
  locations: [DirectiveLocation.FIELD],
});
const schema = new GraphQLSchema({
  directives: [UppercaseDirective],
  ...
});

and such Author type with resolve method:

const UppercaseDirective = new GraphQLDirective({
  name: 'uppercase',
  description: 'Provides default value for input field.',
  locations: [DirectiveLocation.FIELD],
});

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: {
      type: GraphQLString,
      resolve: (source, args, context, info) => {
        if (info.fieldNodes?.[0].directives?.[0]?.name?.value === 'uppercase') {
          return source.name.toUpperCase();
        }
        return source.name;
      },
    },
  }),
});

For now Directives are quite stupid and adds only some meta to infoAST argument. You need manually traverse fieldNodes in your resolve method for checking directive name by string. (BTW its very strange that info does not contain Directive instance for fast comparision which cheked on query validation step, eg info.fieldNodes?.[0].directives?.[0]?.instance === UppercaseDirective).

Anyway with current directive implementation it does not make sence adding directives to FieldConfig.

nodkz commented 5 years ago

BUT what if can use directive as middleware?

For example, we create a new kind of directive:

const ChangeCaseDirective = new GraphQLMiddlewareDirective({
  name: 'changecase',
  // locations: [...], // no need, because this directive works only `DirectiveLocation.FIELD` level
  args: {
    fieldName: { type: new GraphQLNonNull(GraphQLString) },
    upper: { type: new GraphQLNonNull(GraphQLBoolean) },
    lower: { type: new GraphQLNonNull(GraphQLBoolean) },
  },
  resolve: async (directiveArgs, resolve, source, args, context, info) => {
     const { fieldName, upper,  lower} = directiveArgs;
     let value = await resolve(source, args, context, info) || source[fieldName];
     if (typeof value === 'string') {
        if (directiveArgs.upper) value = value.toUpperCase();
        else if (directiveArgs.lower) value = value.toLowerCase();
     }
     return value;
  },
});

const schema = new GraphQLSchema({
  directives: [ChangeCaseDirective],
  ...
});

This directive can be defined only on schema build step via SDL or ObjectType constructor. It cannot be defined in GraphQL query, otherwise

So with such directive we can custruct type in such way:

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: { type: GraphQLString },
    lowerName: { 
      type: GraphQLString,
      directives: [
        [ChangeCaseDirective, { fieldName: 'name', lower: true} ]
      ],
    },
    upperName: { 
      type: GraphQLString,
      directives: [
        [ChangeCaseDirective, { fieldName: 'name', upper: true} ]
      ],
    }, 
  }),
});

With such case, it is almost similar to @maticzav aproach in graphql-middleware.

And maybe no reason to make such GraphQLMiddlewareDirective. Cause with graphql-middleware it can be done in the more convenient way with better static analysis:

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: { type: GraphQLString },
    lowerName: { 
      type: GraphQLString,
      middlewares: [
        changeCase({ fieldName: 'name', lower: true }),
      ],
    },
    upperName: { 
      type: GraphQLString,
      middlewares: [
        changeCase({ fieldName: 'name', upper: true }),
      ],
    }, 
  }),
});

@leebyron It will be great if graphql will have the ability to provide middlewares for resolve methods out of the box. Cause it helps to keep resolve methods thin and enabling code reuse and a clear separation of concerns (for query cost, permission rules, logging, error tracking and other useful methods).

nodkz commented 5 years ago

With current implementation of resolve method applying several middlewares looks aweful:

const changeCaseMW = changeCase({ fieldName: 'name', lower: true });
const queryCostMW = queryCost({ ...someSettings });

const AuthorType = new GraphQLObjectType({
  name: 'Author',
  fields: () => ({
    id: { type: GraphQLInt },
    name: { type: GraphQLString },
    lowerName: { 
      type: GraphQLString,
      resolve: async (source, args, context, info) => {
        return queryCostMW( 
           () =>changeCaseMW(
               () => { return ...somehow resolved data... },
               source, args, context, info
            ),
            source, args, context, info
        );
      },
    },
  }),
});
MichalLytek commented 5 years ago

I assume the Apollo tools translate these GraphQL language directives to some other kind of metadata on the underlying schema and type objects - it would be best to use whatever API the apollo tools expect directly.

@leebyron For me it looks like they read the directives from AST directly: https://github.com/apollographql/apollo-server/blob/ae9da10e625cf283568ba6d29cea8c3e69a7a03f/packages/apollo-cache-control/src/index.ts

So there's no way to pass them as an GraphQLObjectType constructor argument option, like a cacheControl field or something like that. graphql-query-complexity use this way of attaching metadata to types:

const Query = new GraphQLObjectType({
  name: 'Query',
  fields: () => ({
    posts: {
      type: new GraphQLList(Post),
      complexity: ({args, childComplexity}) => childComplexity * args.count,
      args: {
        count: {
          type: GraphQLInt,
          defaultValue: 10
        }
      }
    },
  }),
});

I get your point that when we are responsible for executing the schema, we don't need to add metadata by directives as we can just use kinda middlewares in resolvers.

But for integrating our schema with 3rd party tools, I believe that it's better to expose directives as the most developer friendly way, rather than forcing everybody to dig into the source code of the package.

mattkrick commented 5 years ago

Apollo sticks it on theastNode directly, whereas the pattern in @nodkz's example puts it on the field.

The difference in implementation is pretty trivial, but enough to break it. For example, here's what a @rateLimiter directive looks like using the IDL: https://github.com/withspectrum/spectrum/pull/2874/files#diff-2a03f207138e321cfd01f51e50800816R82

And here's how the same thing looks using JavaScript objects instead of the IDL: https://github.com/ParabolInc/action/commit/5bc77a3df9815ec53cfcd90a39f331721d1f91a8#diff-41b9790b1ca42a3ee29cfeb3ccc31713R16

Frankly, I think it's an error that directives are put directly on the astNode & that Apollo's parser should instead assign it to the field, but I'm often wrong :man_shrugging:

FWIW, over the last couple years I've implemented a handful of custom directives & each time I do, I regret it. The logic usually fits much better down the chain in a HOF around resolve or up the chain in the request's context.

For example: resolve: rateLimit({perMinute: 10})(...} is just so much cleaner than directives: [{name: 'rateLimit', args: {perMinute: 10}}].

MichalLytek commented 5 years ago

FWIW, over the last couple years I've implemented a handful of custom directives & each time I do, I regret it. The logic usually fits much better down the chain in a HOF around resolve or up the chain in the request's context.

And nobody denies it. Support for registering directives is only for 3rd party libs like apollo-cache-control that use IDL and read directives from AST: https://github.com/apollographql/apollo-server/blob/ae9da10e625cf283568ba6d29cea8c3e69a7a03f/packages/apollo-cache-control/src/index.ts

All custom code can be implemented by HOC, middlewares, and custom properties in ObjectType config.

IvanGoncharov commented 5 years ago

The solution for this is discussed in #1527 @nodkz We planning to include extensions in 14.2.0 release. Can you please review #1527 and comment on whenever it solve your issues?

MichalLytek commented 5 years ago

@IvanGoncharov It's not a solution for this. It's only for decorators-like behavior for custom JS code, like placing the metadata and reading it in resolvers (middlewares).

We still need a way to register directives that are present in printSchema and available in astNode because some 3rd party packages read the directives from AST directly (apollo-cache-control). And for https://github.com/19majkel94/type-graphql/issues/217 it would be great to be able to print a schema with directives generated by graphql-js JS API.

IvanGoncharov commented 5 years ago

because some 3rd party packages read the directives from AST directly (apollo-cache-control).

As an author of #746 that added astNode in the first place, I can say that initially I also wanted to expose appliedDirectives on every type, field, enum value, etc. So then Lee rejected this proposal with this comment: https://github.com/graphql/graphql-js/pull/746#issuecomment-301554231

Right now directives are purely a feature of the GraphQL language and IDL, so a schema not created using IDL definitionally won't contain directives. When building a schema in code, you have the full power of the host programming environment, so directives shouldn't be necessary. Directives are a tool for supplying additional intent to be later interpreted by the host programming environment.

As for exposing more information from introspection - that's a bit out of scope for this particular change, and that RFC may go through more changes. I see type metadata and IDL directives as likely two related but not identical concepts, and don't see a need to prematurely link the two for this particular change.

I didn't fully understood it but was happy that he would at least agree to add astNode that was better than nothing. After working more with GraphQL and helping to maintain both spec and graphql-js I now fully agree with Lee point, but I feel like we never really explain it properly. It would be great to post here URL to some article or design document that explains it better, but I never saw any such document ๐Ÿ˜ž So I will try my best (it's late night ATM + I'm not a native speaker) to explain it here:

Directives were added as an extension point for creating new syntax, and they were never intended to be a part of the type system since attaching metadata is not the only use case for directives and in many cases directives are used to create completely new syntax, e.g.: https://github.com/OlegIlyenko/graphql-gateway/blob/master/testSchema.graphql Another example is @import discussed here: https://github.com/facebook/graphql/issues/410

I think we can all agree that such directives make sense only during schema building and shouldn't be present in result schema. It would be even more confusing if such directives would reappear when printing result schema back to SDL.

For a long time, I thought that the correct solution would be to whitelist directives that should be passed to the schema and ignore the rest, but after several years I understood that this workaround wouldn't solve the fundamental issue: On the one hand, we want type system to be as strict as possible to make it easy to work with (in middlewares or in any other tool). On the other hand, we want directives to be as flexible as possible so 3rd-party developers can experiment with new syntax without forking GraphQL parser and creating new GraphQL dialect.

To explain it closer to code lets take my initial version of #746 and imagine it was merged as is and all schema object have appliedDirectives as the map with a directive name as key and directive args as value. It would be easy to work with and fully compatible with a spec at that time, but since then we discover a few new use cases for directives:

  1. https://github.com/facebook/graphql/pull/470 to implement directive chains like this:
    type Query {
    users: [User]
    @httpGet(url: "...")
    @jsonPath(path: "$.results")
    @enrichData
    @jsonPath(path: "$.profile")
    }
  2. https://github.com/facebook/graphql/pull/472 That allow to duplicate directives on the same location

Both these changes are fit original intention for directives so ATM they are on a fast track to be included in the next spec release. But they break my origin idea for appliedDirectives since we can't duplicate keys and we can't preserve key order.

@nodkz proposal of serializing directives as an array of objects with name and args properties address both issues but there is no guarantee that it will fit nicely with all future spec addition for directives, e.g., top-level directives (https://github.com/facebook/graphql/issues/410) that are different from schema directives so they can't be attached to schema or any other object inside schema.

There are many other potential problems, but I don't want to convert this comment into the blog post :)

@19majkel94 Not sure that above explanation clearly explains my reasoning so I will also try to use the analogy from your domain. Please correct me if I'm wrong since I never used JS decorators myself but AFAIK they are functions executed during object creation, and you can't access their args after the object was created. But JS decorators are free to do any transformation including adding new properties.

Imagine how restrictive it would be if instead of calling a function JS decorators would add an item to decorators array defined on the appropriate object. Plus it would be very easy to lose such decorator:

class Test {
  @someDecorator
  test() {
  }
}

const modifiedTest = Test.prototype.test.bind(...)

Should modifiedTest retain decorators that were attached to the test method?

Similar to JS decorators that make sense only as syntax sugar during class creation, GraphQL directives are syntax sugar that should be applied only during SDL => GraphQLSchema phase. And similar to JS decorators that can add additional properties, GraphQL directives can write to extensions.


Hope now you understand why I strongly oppose adding applied directives to GraphQL schema even though I was promoting this idea just a few years ago.

We still need a way to register directives that are present in printSchema and available in astNode because some 3rd party packages read the directives from AST directly (apollo-cache-control).

Ideally, 3rd party tools should use astNodes from user-provided schema (if it passed as GraphQLSchema) only for error reporting and nothing else. ATM schema transformations becoming increasingly more popular and it's even more concerning since during transformation schema is heavily modified, but astNode stays unchanged.

I think extensions (discussed in #1527) is a good long term solution. Ideally, tools like apollo-cache-control should provide schema transformation that you would use on SDL => Schema phase to convert directives from astNodes into extensions.

This is a better solution since you can always write JS code that transforms any syntax sugar you invented (top level directives, directive chains, duplicated directives) into a single extensions property that has no restrictions on its value (with directives you always restricted to directive args).

Plus extensions are way easier to use programmatically since you can do queryType.extensions.apolloCacheControl instead of queryType.directives.find(dir => dir.name === 'apolloCacheControl').

And for 19majkel94/type-graphql#217 it would be great to be able to print a schema with directives generated by graphql-js JS API.

We can always add some mechanism (e.g., callback) to printSchema that will convert extensions into a set of directives on appropriate node.

P.S. I also working on spec RFC to add extensions into introspection since it next logical step for exposing user-defined metadata SDL => Schema => Introspection => ClientSchema. But it still at the early stage so I'm not ready to publish it ATM.


@nodkz If you have time read this super long comment it would be great to hear your opinion on the topic.

nodkz commented 5 years ago

@IvanGoncharov extensions on fieldConfig level looks good for me and for graphql-compose ๐Ÿ‘

BUT we should consider unification of directives (on field level at schema definition) and extensions (on field config level).

Consider using a new proposed extension with graphql-compose:

// add field via FieldConfig
UserTC = TypeComposer.create('User');
UserTC.addField('name', {
  type: 'String',
  extensions: {
    cost: { complexity: 2 },
  }
});

// create type via SDL
ArticleTC = TypeComposer.create(`
  type Article {
    title @cost(complexity: 2)
  }
`);

The question is: Can be schema directives and extensions unified somehow? If we not unify them, or not provide migration procedure from schema directives to extensions, then we split graphql world quite more than now. ๐Ÿคข


Let's make one step back.

Clients

When clients need to tune resolver behaviour then may use args. So backender can read the second argument in resolve method and provide such behavior.

Backenders

For now become quite popular to use middlewares for resolvers. It helps to drastically reduce amount of resolvers code. But need to have a nice way to provide such "backenders" arguments from schema to the middlewares.

For now backenders cannot setup any args on schema level. In SDL they found a hack via diretives on schema construction. In field config way there are not such ability. And extensions will add such ability.

So we need to think how can be unified using of SDL directives and extensions for middleware authors. Eg. via a new property info.fieldExtensions or in some other way:

resolve(source, args, context, info) {
  info.fieldExtensions
}

Also we should not forget about modularity and schema stitching. Some part of schemas may be realized via graphql-tools way, other via graphql ConfigObjects.

It will be nice to have such implementation which helps to use backenders args in runtime. Current directive info.fieldNodes?.[0].directives?.[0]?.name?.value === 'uppercase' implementation provides too much runtime checks.

Anyway we need to take a fresh look on problem of backenders' arguments for resolve methods. And solution for this problems shows us a way how to implement extensions and unify them with current directives in SDL.

Thoughts?

nodkz commented 5 years ago

@IvanGoncharov you may ping me via Telegram by nickname @nodkz or via Skype nodkz1985 and we can closely discuss this problem.

taion commented 5 years ago

I'll also note that some of the concerns around metadata are specifically around communicating more information to clients. A canonical example here is augmenting type system information by informing the client of e.g. min and max values for numeric inputs.

MichalLytek commented 5 years ago

@IvanGoncharov Great explanation! ๐Ÿ‘

Now I get your point of view and agree with that - it makes no sense to add weird things just to match with 3rd party libs that are using workarounds. It's better to propose a well designed API/specification and change those libs to support that ๐Ÿ˜‰

And I agree with @nodkz that we need some kind of unification of simple metadata-like directives with the extensions, so we can support both ways of registering additional data with simple way of handling the related logic (like cost and complexity).

freiksenet commented 5 years ago

At Gatsby we've been using graphq-compose type/field extะตnsions. We also use directives to allow setting extension metadata through that. I feel that having a distinct namespaces for directives (within or outside extensions), that is a list of directives with arguments applied to type/field/argument is the way to go here. This matches the behavior between AST and extensions (eg you can apply same directive multiple time). If extensions would also be exposed through introspection, this allows inspecting them too (again, exactly in the way they are defined in SDL).

type Foo @directive1(arg1: "bar") {
  id: ID! @id
  string: String @validation(regex: "/abc+/") @required
}

> Foo.extensions

{
  directives: [
    {
      name: "directive1",
      args: {
        arg1: "bar"
      }
    }
  ]
}

> Foo.getField('string').extensions

{
  directives: [
    {
      name: "validation",
      args: {
        regex: "/abc+/"
      }
    },
    {
      name: "required",
      args: {},
    }
  ]
}
nodkz commented 5 years ago

@freiksenet your approach definitely SHOULD land to the core of graphql-compose

Even more for easy type manipulation, I'll add helper methods, like

FooTC.getDirectiveByName('directive1'); // { arg1: "bar" }
FooTC.getDirectiveByName('directive2'); // undefined
FooTC.getDirectiveNames(); // ['directive1']

FooTC.getFieldDirectiveByName('fieldName', 'validation'); // { regex: "/abc+/" }
FooTC.getFieldDirectiveByName('fieldName', 'required'); // {}
FooTC.getFieldDirectiveNames('fieldName'); // ['validation', 'required']

// For users who use multiple directives with same names
FooTC.getFieldDirectiveById('fieldName', 0); // { name: "validation", args: { regex: "/abc+/" }}
FooTC.getFieldDirectiveById('fieldName', 1); // { name: "required", args: {}}
FooTC.getFieldDirectiveById('fieldName', 2); // undefined

These methods help to simplify the code of end users.

mike-marcacci commented 4 years ago

I think that @IvanGoncharov's explanation above is absolutely fantastic and clarified some misconceptions I held about the intended purpose of directives. With this in mind I believe it's quite clear that the ability to list directives in a GraphQLObjectType is completely superfluous.

However, I believe that this issue gets at a more fundamental question: "How does one store and propagate generic metadata in GraphQL?"

This isn't really a new question, and is currently not solved in a generic sense by the spec. However, the spec does solve this problem for one specific use case: deprecation.

This case clearly shows the separation between the concerns of directives and the concerns of metadata: the graphql-js library includes a @deprecated(reason: String!) directive which sets the corresponding metadata on the created type. This metadata is not propagated by continued presence of the directive, but by the isDeprecated and deprecationReason fields that are set by the directive.

There are two further distinctions within the "metadata" concern: propagation of metadata inside the application, and propagation to an outside system via an SDL document or introspection query. In the case of deprecation, both of these concerns are met with special-case fields on both the js objects and the introspection types; and for serialization to SDL we can reconstruct the @deprecated directive.

For custom metadata propagation, we have the extensions property in graphql-js. For tools that expect an SDL, we may be able to reconstruct custom directives; for those that make an introspection query, we can extend the relevant introspection type.

That last part isn't particularly obvious or elegant with graphql-js, but is certainly possible. For example, extending the __Field type with an isExperimental flag:

__Field.getFields().isExperimental = {
  type: new GraphQLNonNull(GraphQLString),
  resolve(source) {
    return (source.extensions && source.extensions.isExperimental) || false;
  },
  args: [],
  name: "isExperimental",
  description: undefined,
  isDeprecated: false,
  deprecationReason: undefined,
  extensions: undefined,
  astNode: undefined
};

To me it's now pretty clear that this issue is simply a misconception of directives, conflating them with metadata more generally. I think the real solution here is better documentation on the purpose of directives, and possibly some outreach to Apollo and other teams whose tutorials might be reworded to avoid conveying an incomplete picture of directives.

@MichalLytek do you agree with this? Would you be comfortable with this issue being closed?

EDIT: An additional step may be adding better support to graphql-js for extending introspection types without global ramifications.

MichalLytek commented 4 years ago

@MichalLytek do you agree with this?

I know, that we can transform the schema by code, wrap the resolvers and make other things to apply behavior of directives or store metadata in extenstions and it's not a problem in this issue.

As I said before, the problem with directives is a compatibility with existing 3rd party libraries. And it will exist until all the libs will switch from reading the AST looking for directives into reading the extenstions or other form of receiving the metadata by introspection.

For now it's not an urgent issue as @j found a workaround to apply AST directives while still building the schema by code. We should discuss a universal solution for that use cases, where directives are only on top of that for SDL approach of creating the schema.

wtrocki commented 4 years ago

However, I believe that this issue gets at a more fundamental question: "How does one store and propagate generic metadata in GraphQL?"

The community created a couple of packages to support that without any change in the GraphQL reference impl. All those solutions base on the fact that parser puts comment information into description field in the schema.

Example of the library that reinvents some of the concepts: https://github.com/aerogear/graphql-metadata

EDIT: I'm aware that this is not the solution or addresses any concerns of the original issue, but basically I personally feel that SDL was never intended to keep anything else than just GraphQL related information.

However, as GraphQL started becoming more and more popular many developers wanted to collocate their metadata closer to the schema. The best way to handle this is to build your own preprocessor on top of the SDL or rely on description field which actually intends to provide human-readable info (but I can provide also machine-readable as well)

tot-ra commented 3 years ago

My use case is.. We have service with schema composed with GraphQLObjectType objects, now if I want to make it Apollo-federated, I need to add directives (like @external), so I'm making SDL out of these objects with printSchema, but I don't see a way to define these directives on objects. (Do I need to use extensions + make custom printSchemaWithDirectives?

Directives are purely a feature of the GraphQL language and are not part of type definitions - this is because different tools will use and interpret directives in different ways.

Since a schema is an in-memory data structure artifact, and not simply a representation of the GraphQL language, it does not have a concept of directives on its own. https://github.com/graphql/graphql-js/issues/869

To quote the spec - "Directives can be used to describe additional information for types, fields, fragments and operations.". If this project is a "reference implementation for GraphQL", then shouldn't I be able to define directive on a field as spec states? If I can use it in SDL, why I can't do this programmatically with an object definition?

If argument is about strict types & interpretation, then language has scalars.. those are interpreted differently. And even GraphQL type system is also vague, for example Integer can be unsigned, 32-base or 64-bit..

Like @mike-marcacci mentioned, if @deprecated directive manifests in deprecationReason and isDeprecated.. why this is not part of a more general directives interface? Because it was the main use-case for directives? Directives could be in-memory structure too with any type if there is some reasoning for performance/stability/simplicity.

yaacovCR commented 3 years ago

For what it's worth, graphql-tools now reads "directive" metadata from extensions by default when present instead of AST following graphql-compose/Gatsby convention above.

We don't yet write directives to extensions, as unclear which directives are metadata...

rattrayalex commented 3 years ago

I'd like to be able to construct a gql schema with a code-first tool like typegraphql, print the schema, and see what I constructed.

For example, I'd like to be able to write this:

@ObjectType()
class Bar {
  @Directive("@auth(requires: USER)")
  @Field()
  field: string;
}

and get this:

type Bar {
  field: String! @auth(requires: USER)
}

however, that isn't possible today, due to this issue.

Sure, my directives will work fine at runtime, but I'd like to be able to look at my generated schema.graphql and see which fields require auth, or which are deprecated, or which have a high cost, or whatever.

It seems there's some reticence to add support for storing directives because runtime tooling should ideally handle the functionality of directives in another manner, but this would be useful outside of runtime behavior, like looking at a textual schema with human eyeballs or a linter.

sescotti commented 1 year ago

hi, this ticket has been open for 5 years already, is it safe to assume it's not going to be worked on?

I have a similar situation to @rattrayalex, I'm using AWS AppSync and I need to set some auth directives that are enforced by the cloud service, so I don't need to implement them myself, just declare them.

using a code-first tool like type-graphql would help to circumvent the limitations of defining a gql file directly, but this is a deal breaker.

thanks,.

y-nk commented 4 months ago

i'm interested into knowing how to bring those directives back. I understand it's not a bug, but it's a soft requirement in order to compute a superschema with rover (from apollo).

The process with using Apollo Studio includes publishing a schema to their platform with their cli, which uses a schema file. The said file is the .graphql which then is sent to their cloud for superschema resolution (and conflicts).

The issue appears when a schema doesn't have the @key or @extends directive because a shared Object type is seen as competing (in the absence of @extends/@shareable) and the superschema fails to compute... and therefore, there's a need for the schema to have these directives printed on the schema.

I'm not sure this is the best place, as i totally understand it is implementation dependant. Pothos has a working one, while Nest.JS does not. But i wish to ask from here what's the best course of action regarding graphql spec rather than anything else.

Is Apollo rightful in their way? Is Pothos implementation good or Nest.JS rather following it? Should we really not have directives in the schema? If not, how could we ever resolve a superschema with schemas having directives missing?

Thanks a lot and sorry for the long message