Skip to content

Directives at the top of a SDL file #410

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

Open
sorenbs opened this issue Feb 26, 2018 · 5 comments
Open

Directives at the top of a SDL file #410

sorenbs opened this issue Feb 26, 2018 · 5 comments
Labels
👻 Needs Champion RFC Needs a champion to progress (See CONTRIBUTING.md) 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)

Comments

@sorenbs
Copy link

sorenbs commented Feb 26, 2018

In the latest Working Group meeting Graphcool presented GraphQL Import. It was brought up that we could change the syntax to be based on directives instead of comments:

@import(defs: [Post, Comment] from: "./other-types.graphql")

instead of

# import Post, Comment from "./other-types.graphql"

In the current spec, directives must always be placed after another node. As such, there is no way to place a directive at the top of a file. I volunteered to create an RFC to rectify this using the @import directive as motivation.

Discussion in the issue for graphql-import let to the conclusion that import should not switch to a directive based syntax.

I still think it might be valuable to allow directives in the beginning of a file and would love to hear other use cases for this before proceeding with an RFC.

@leebyron
Copy link
Collaborator

leebyron commented Mar 6, 2018

I definitely support this proposal. I've seen plenty of use cases in related languages:

Headers in Thrift used for both imports and describing code generation namespacing details. https://thrift.apache.org/docs/idl#header

Protobuf has a similar set of explicit grammar rules for this top level metadata https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#proto_file

Capn Proto has a generic grammar similar to directives called annotations (better name IMHO, whoops) that it describes having many uses including code generation control https://capnproto.org/language.html#annotations

@benjamn
Copy link

benjamn commented Mar 22, 2018

If we didn't have top-level directives, you could still do a lot with type-level @import directives:

type Post @import(from: "./other-types.graphql") {}
type Comment @import(from: "./other-types.graphql") {}

Since you're importing into the Post and Comment types here, you wouldn't have to specify what to import in the arguments to the @import directive. The implementation of @import could infer what to import by examining the name of the type where the @import directive was found. In other words, there's no need for the defs: [Post, Comment] argument.

Presumably you could also rename types imported from the sub-schema, using other optional @import arguments, but I suspect that would not be necessary most of the time.

If we did have top-level directives, I imagine they would be equivalent to directives on the schema declaration, since a top-level schema directive should naturally operate on the GraphQLSchema object:

schema @import(...) {
  query: Query
  mutation: Mutation
}

Either way, there's a problem here, because anyone coming from other languages will probably assume they can have more than one top-level @import directive, but that's currently forbidden by the directive uniqueness rules. Should we relax that restriction?

Another problem with using directives for imports: while it makes composing SDL documents a little easier, it doesn't help you organize your resolver functions in a similar way. Because you're no longer manually creating the ./other-types.graphql schema, you don't have an opportunity to specify the resolver functions for that sub-schema until everything is combined into the final schema, so it's difficult to colocate your resolver functions with their corresponding sub-schemas.

I'm interested in implementing imports without using comments, but I'm skeptical that directives are the way to do it.

@schickling
Copy link

Hmm this definitely looks like an interesting idea, @benjamn!

I've recently had a chat with @jnwng and @gagoar about the same topic. Their preferred syntax seemed to have been the following:

@import(from: "./other-types.graphql" defs: [
  "Post",
  "Comment"
])

@jnwng @gagoar would love to hear how your thoughts have evolved.

@RemyRylan
Copy link

RemyRylan commented Apr 1, 2018

I definitely support the syntax that @schickling is proposing, putting the from source first is much better for autocomplete/intellisense hinting in IDEs.

Unrelated but it's such a shame that JS ES6 follows the format of definitions followed by source... A lot of GraphQL developers may find themselves accustomed to this pattern but I feel there's no need to repeat mistakes in other languages that developers are having to constantly workaround just because it's the norm.

Case in point: I find myself constantly typing JS imports in the following fashion:

Step 1:

import {} from 'some_source'

Step 2:
Move my cursor back to the definitions statement of the import and type out whatever I need so that my IDE can provide intellisense for the import source:

import { something } from 'some_source'

@JeffRMoore
Copy link

OpenAPI/Swagger/JSON Schema use JSON Reference for the import case.

I think import is worthy of its own syntax rather than piggybacking on directives. A GraphQL document with imports won't validate without understanding the imports, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👻 Needs Champion RFC Needs a champion to progress (See CONTRIBUTING.md) 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

No branches or pull requests

6 participants