Add support for service capabilities#6858
Conversation
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: 2a5885ec89f213c74fb4619d |
martinbonnin
changed the title
| * TODO: This is potentially an issue because it means adding new features changes the output of this function. | ||
| * For an example, it doesn't support service capabilities as of now to avoid confusing other external tools. |
There was a problem hiding this comment.
I'm not sure why we need the SDL -> JSON transformation. For interop, this is an issue because different tools may not support all the introspection features.
I decided to not add support for service capabilities there and more generally moving forward, probably deprecating this functionality.
There was a problem hiding this comment.
Agreed this should probably be deprecated, I can't think of a use-case for it. Ah except maybe to generate fixtures for our own tests? Meh.
There was a problem hiding this comment.
Yea, if the only usage is tests, this is not enough. People may be using the CLI to convert to JSON, not 100% sure why they would do this but that's a possibility.
There's no rush. We can keep it untouched for now.
|
|
||
| expectKeyword("service") | ||
| val directives = parseDirectives(const = true) | ||
| val capabilities = parseNonEmptyList<Token.LeftBrace, Token.RightBrace, GQLCapability> { |
There was a problem hiding this comment.
According to the spec the list can be empty, but actually I think the spec should be + because an empty block doesn't make much sense?
There was a problem hiding this comment.
Ah, yes, I remember that discussion. There are multiple issues of people wanting to have empty objects/selection sets and IIRC, @benjie used capabilities as a place to toy with empty blocks. I'll update that code so that it matches the current PR.
There was a problem hiding this comment.
Specifically, Lee said we should allow empty blocks here.
| parseServiceCapability() | ||
| }.orEmpty() | ||
|
|
||
| if (directives.isEmpty() && capabilities.isEmpty()) { |
There was a problem hiding this comment.
Same here, this makes sense but doesn't match the current spec.
There was a problem hiding this comment.
Good point. I'll keep this in line with the current spec PR but while I think it makes sense for the definition, I'm not sure about the extension. Asked the question there
| * TODO: This is potentially an issue because it means adding new features changes the output of this function. | ||
| * For an example, it doesn't support service capabilities as of now to avoid confusing other external tools. |
There was a problem hiding this comment.
Agreed this should probably be deprecated, I can't think of a use-case for it. Ah except maybe to generate fixtures for our own tests? Meh.
Co-authored-by: Benoit 'BoD' Lubek <BoD@JRAF.org>
Co-authored-by: Benoit 'BoD' Lubek <BoD@JRAF.org>
3 participants
See graphql/graphql-spec#1208
The parser has good tests. Introspection/execution still need a bit more tests but since this is RFC1, I expect things to change a bit and I don't want to spend too much time updating every test.