[RFC] Document Directives #556

langpavel · 2019-01-31T17:53:30Z

This addresses #410 Directives at the top of a SDL file

WIP

✔️ Required grammar and introspection changes included

IvanGoncharov · 2019-01-31T18:39:56Z

spec/Section 2 -- Language.md

+
+```graphql example
+@import(from: "./common")
+@import(from: "./user")


I don't think we should use @import directives as an example here.
Since some readers can think it a standard one and supported by GraphQL spec.
I think it would be better to use something like @someTopLevelDirective, @foo, @test, etc.

This is most relevant example I wish to show and indeed it is my intent to push on some common way to do import. I can add note or warning below that import is not built-in

As @import will have meaning in a bunch of libraries almost immediately, and those meaning will probably immediately diverge, let's keep it out of the spec entirely. There aren't any descriptions of any other user-level directives in the spec.

The only other usage is @addedDirective and @example because they illustrate the usage without introducing a new concept.

In this case, you could call it @exampleDocumentDirective or even just @example

IvanGoncharov · 2019-01-31T18:43:39Z

spec/Section 2 -- Language.md

+```
+
+Unknown directives may be ignored, but it is recommended for services to fail
+if directive is not recognized.


@langpavel It contradicts rules for all other directives:
https://facebook.github.io/graphql/June2018/#sec-Directives-Are-Defined

Personally, I think we shouldn't have any exceptions for top-level directives.

So, this sentence can be dropped entirely, ok?

IvanGoncharov · 2019-01-31T18:44:57Z

spec/Appendix B -- Grammar Summary.md

@@ -94,10 +94,14 @@ lines and uniform indentation with {BlockStringValue()}.
 Document : Definition+

 Definition :
+  - DocumentDirective


Suggested change

- DocumentDirective

- DocumentDirectives

No, DocumentDirective (singular) is really what should be stated in grammar.

I see now
Then I think it makes more sense to do

Document: Directives[Const] Definition+

This will prevent you from inserting document level directives into middle of document.
This may be problematic if you concatenate more documents into one.

Moreover there is chicken egg problem, when you wish define document directive you cannot use it in same document

Moreover there is chicken egg problem, when you wish define document directive you cannot use it in same document

You can. Why not?
GraphQL doesn't care about order of things in SDL. This is perfectly valid:

scalar SomeScalar @someDirective directive @someDirective on SCALAR

Oh, you are right…

IvanGoncharov · 2019-01-31T18:50:05Z

spec/Section 2 -- Language.md

@@ -1094,3 +1095,24 @@ and operations.

 As future versions of GraphQL adopt new configurable execution capabilities,
 they may be exposed via directives.
+
+
+## Document Directives


Don't think they should be a special case of directives.
They should be covered by: https://facebook.github.io/graphql/June2018/#sec-Language.Directives

Intent is place document level directives aside from others, there apply different rules:

Document directives may be repeated, others not.

Document level directives are top level AST Document DocumentNode.definitions nodes, other directives are listed in XxxNode.directives — this may need discussion

Document directives may be repeated, others not.

#472 add support of reputable directives.

Document level directives are top level AST Document DocumentNode.definitions nodes, other directives are listed in XxxNode.directives — this may need discussion

I think DocumentNode.directives makes more sense.

Yeah, I'd much rather see this compose with the repeatable directives change.

Moreover, even if we never gained repeatable directives, document-level driectives (such as @import) could always be expanded to take lists instead @imports(from: ["foo", "bar/baz"]). It's better to keep the total number of concepts down: if something looks like it should work in two contexts, but subtly works a different way in one of them, that's a spec bug.

IvanGoncharov · 2019-01-31T19:25:10Z

I think we need to have a separator between top-level directives and rest of SDL file

@import(from: "./common")
@import(from: "./user")
---

type Query {
  foo: String
}

Similar to YAML: https://yaml.org/spec/1.1/#YAML%20directive/

Because without it we can cause some confusion:

@someDirective

scalar Url

@someDirective is top level directive but some user may think that @someDirective belongs to scalar definition especially since it looks very similar to decorators in JS and many other languages.

Plus I saw some people concatenate SDL files so if you have two files like this:

scalar SomeScalar

and

@someDirective

scalar AnotherScalar

We would get valid SDL:

scalar SomeScalar
@someDirective

scalar AnotherScalar

But parser will parse @someDirective as part of SomeScalar definition.
With 3 dashes we will have:

scalar SomeScalar
@someDirective
---
scalar AnotherScalar

The parser will error saying you can't use --- in the middle of the file.

If we decide to allow top-level directives we can always allow:

scalar SomeScalar

---
@someDirective
---

scalar AnotherScalar

langpavel · 2019-01-31T20:33:45Z

Hmm, good point with grammar ambiguity, however I don't like --- separator,
What about new keyword document followed by document level directives?

mjmahone

There's another question: most directives are either executable or schema directives: you can't use a schema-level directive inside an executable document, and vice versa. Which of these is the document directive? Is it neither? Why?

Basically: if I have a document with a bunch of fragments, should I be able to include a directive that modifies the schema? Why or why not?

mjmahone · 2019-01-31T23:59:37Z

spec/Section 2 -- Language.md

@@ -1094,3 +1095,24 @@ and operations.

 As future versions of GraphQL adopt new configurable execution capabilities,
 they may be exposed via directives.
+
+
+## Document Directives


Yeah, I'd much rather see this compose with the repeatable directives change.

Moreover, even if we never gained repeatable directives, document-level driectives (such as @import) could always be expanded to take lists instead @imports(from: ["foo", "bar/baz"]). It's better to keep the total number of concepts down: if something looks like it should work in two contexts, but subtly works a different way in one of them, that's a spec bug.

mjmahone · 2019-02-01T00:03:45Z

spec/Section 2 -- Language.md

+
+```graphql example
+@import(from: "./common")
+@import(from: "./user")


As @import will have meaning in a bunch of libraries almost immediately, and those meaning will probably immediately diverge, let's keep it out of the spec entirely. There aren't any descriptions of any other user-level directives in the spec.

The only other usage is @addedDirective and @example because they illustrate the usage without introducing a new concept.

In this case, you could call it @exampleDocumentDirective or even just @example

Changes made make it no longer applicable

Added DOCUMENT directive location + example

893e77d

facebook-github-bot added the CLA Signed label Jan 31, 2019

Document Directives - grammar plus description

3a86550

langpavel force-pushed the document-directive branch from 37cbfc9 to 3a86550 Compare January 31, 2019 17:57

IvanGoncharov reviewed Jan 31, 2019

View reviewed changes

IvanGoncharov added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Jan 31, 2019

mjmahone previously requested changes Feb 1, 2019

View reviewed changes

Document directives are not special

f89de40

leebyron removed the CLA Signed label Aug 7, 2019

ghost mentioned this pull request Nov 12, 2019

Subgraph composition graphprotocol/graph-tooling#392

Merged

Base automatically changed from master to main February 3, 2021 04:50

leebyron force-pushed the main branch 4 times, most recently from e5d241d to 6c81ed8 Compare April 23, 2021 19:15

leebyron force-pushed the main branch from de2a1ff to 51337a9 Compare October 26, 2021 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Document Directives #556

[RFC] Document Directives #556

langpavel commented Jan 31, 2019 •

edited

Loading

IvanGoncharov Jan 31, 2019

langpavel Jan 31, 2019

mjmahone Feb 1, 2019

IvanGoncharov Jan 31, 2019

langpavel Jan 31, 2019

IvanGoncharov Jan 31, 2019

langpavel Jan 31, 2019

IvanGoncharov Jan 31, 2019 •

edited

Loading

langpavel Jan 31, 2019

langpavel Jan 31, 2019

IvanGoncharov Jan 31, 2019

langpavel Jan 31, 2019

IvanGoncharov Jan 31, 2019

langpavel Jan 31, 2019

IvanGoncharov Jan 31, 2019

mjmahone Jan 31, 2019

IvanGoncharov commented Jan 31, 2019

langpavel commented Jan 31, 2019

mjmahone left a comment

mjmahone Jan 31, 2019

mjmahone Feb 1, 2019

[RFC] Document Directives #556

Are you sure you want to change the base?

[RFC] Document Directives #556

Conversation

langpavel commented Jan 31, 2019 • edited Loading

WIP

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

IvanGoncharov Jan 31, 2019 • edited Loading

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

IvanGoncharov commented Jan 31, 2019

langpavel commented Jan 31, 2019

mjmahone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

langpavel commented Jan 31, 2019 •

edited

Loading

IvanGoncharov Jan 31, 2019 •

edited

Loading