Add experimental support for parsing variable definitions in fragments #1141

sam-swarr · 2017-12-13T19:45:35Z

This PR adds experimental support for parsing variable definitions within a fragment declaration. If the fragmentVariablesExperimental flag is passed into the options for parse then the parser will accept VariableDefinitions placed after the TypeCondition and before any Directives. The syntax is identical to the usual query-defined variable definitions, and the parser will parse these into the optional variableDefinitions field on FragmentDefinitionNode.

Context:
There's been ongoing discussion on exploring this concept: graphql/graphql-spec#204. Note that this PR only supports parsing fragment variable definitions into the AST. It does not allow for passing variable values into a fragment spread. It at least gets a foot in the door for starting to explore further down this path.

An emergent pain point with the current system (where a query must define all variables used in its dependent fragments) is that adding a new variable reference to a commonly used fragment could involve updating the declarations of the dozens of queries that depend on that fragment. With this PR, it would become easy to implement a client side transform that would allow for variable defaults defined at the fragment level to be automatically hoisted to its dependent queries. For example, with

query q () {
  ...a
}

fragment a on t ($size: Int = 0) {
  f(size: $size)
}

the transform would automatically hoist the $size variable declaration into query q without the need to explicitly define it. This can be useful if fragment a is used by many queries.

mjmahone · 2017-12-13T19:52:39Z

src/language/parser.js

+ /**
+ * If enabled, the parser will understand and parse variable definitions
+ * contained in a fragment definition. They'll be represented in the
+ * `variableDefinitions` field of the FragmentDefinitionNode.


Should we make a point that this is not guaranteed to be supported in the future, and anyone that depends on this may have to migrate away from it at a later date?

Agreed - highlight the fact this is experimental and may change or be removed in the future

leebyron

The implementation looks solid - I think making sure any reader cannot mistake the experiment is important so some suggestions inline

leebyron · 2017-12-13T20:00:14Z

src/language/printer.js

 `fragment ${name} on ${typeCondition} ` +
+ wrap('(', join(variableDefinitions, ', '), ') ') +


Add a comment here about this being experimental for those who stumble upon it

leebyron · 2017-12-13T20:00:27Z

src/language/visitor.js

+ FragmentDefinition: [
+ 'name',
+ 'typeCondition',
+ 'variableDefinitions',


leebyron · 2017-12-13T20:04:22Z

src/language/parser.js

@@ -488,6 +501,17 @@ function parseFragment(
 function parseFragmentDefinition(lexer: Lexer<*>): FragmentDefinitionNode {
 const start = lexer.token;
 expectKeyword(lexer, 'fragment');
+ if (lexer.options.fragmentVariablesExperimental) {


Add a comment above this that includes the altered grammar with a bit of explanation

// Experimental support for defining variables within fragments changes // the grammar of FragmentDefinition: // - fragment FragmentName on TypeCondition VariableDefinitions? Directives? SelectionSet

leebyron · 2017-12-13T20:05:48Z

src/language/parser.js

+ /**
+ * If enabled, the parser will understand and parse variable definitions
+ * contained in a fragment definition. They'll be represented in the
+ * `variableDefinitions` field of the FragmentDefinitionNode.


Agreed - highlight the fact this is experimental and may change or be removed in the future

leebyron · 2017-12-13T20:06:12Z

src/language/parser.js

+ * ...
+ * }
+ */
+ fragmentVariablesExperimental?: boolean,


This option reads a bit strangely, what about experimentalFragmentVariables?

Yeah, I had fragmentVariables_experimental before (not sure if that was any better), but the camelcase linter didn't like that. I'll take your suggestion.

leebyron · 2017-12-13T20:06:46Z

src/language/ast.js

@@ -253,6 +253,7 @@ export type FragmentDefinitionNode = {
 +loc?: Location,
 +name: NameNode,
 +typeCondition: NamedTypeNode,
+ +variableDefinitions?: $ReadOnlyArray<VariableDefinitionNode>,


Add a short comment above this explaining its an experimental addition

leebyron · 2017-12-13T20:07:09Z

src/language/__tests__/visitor-test.js

@@ -290,6 +290,53 @@ describe('Visitor', () => {
 ]);
 });

+ it('visits variables defined in fragments', () => {


it('Experimental: visits variables...

sam-swarr · 2017-12-13T20:26:32Z

Added comments emphasizing the experimental nature of this.

mjmahone

two more experimental-notices and it looks good to me

mjmahone · 2017-12-13T20:43:30Z

src/language/__tests__/parser-test.js

@@ -391,6 +391,16 @@ describe('Parser', () => {
 expect(result.loc).to.equal(undefined);
 });

+ it('allows parsing fragment defined variables if enabled', () => {


nit: Experimental:

mjmahone · 2017-12-13T20:43:40Z

src/language/__tests__/printer-test.js

@@ -103,6 +103,22 @@ describe('Printer', () => {
 `);
 });

+ it('correctly prints fragment definition with variables', () => {


nit: Experimental:

sam-swarr · 2017-12-13T20:48:34Z

Added "Experimental" to the other test cases.

leebyron · 2017-12-13T21:05:18Z

Also - just to discuss I think we have two potential grammars for this experiment that I've seen and we should settle on one with some forethought.

Condition first:
fragment FragmentName on TypeCondition VariableDefinitions? Directives? SelectionSet
Variables first
fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet

I noticed in the linked issue in the graphql spec repo the examples use a variables-first form where this PR is condition first.

Thoughts on one vs the other?

leebyron · 2017-12-13T21:08:12Z

Some examples to look at variables first

fragment ThisIsMyExampleFragment($singleVar: String) on ExampleType {
  withSome
  fields {
    sometimes {
      deep
    }
  }
}

fragment ThisIsMyExampleFragment($singleVar: String) on ExampleType @directive {
  withSome
  fields {
    sometimes {
      deep
    }
  }
}

fragment ThisIsMyExampleFragment(
  $firstVar: String
  $anotherVar: Int
  $evenOneMoreVar: Float
) on ExampleType {
  withSome
  fields {
    sometimes {
      deep
    }
  }
}

Some examples to look at type condition first

fragment ThisIsMyExampleFragment on ExampleType ($singleVar: String) {
  withSome
  fields {
    sometimes {
      deep
    }
  }
}

fragment ThisIsMyExampleFragment on ExampleType ($singleVar: String) @directive {
  withSome
  fields {
    sometimes {
      deep
    }
  }
}

fragment ThisIsMyExampleFragment on ExampleType (
  $firstVar: String
  $anotherVar: Int
  $evenOneMoreVar: Float
) {
  withSome
  fields {
    sometimes {
      deep
    }
  }
}

sam-swarr · 2017-12-13T21:21:38Z

No strong opinions, but I leaned towards the latter (with TypeCondition first) since I like the look of the VariableDefinition's closing parenthesis immediately before the open curly brace of the SelectionSet. It more closely matches how JS functions and if statements look rather than having the variables embedded between two strings. I realize this doesn't hold true if there's a directive present on the FragmentDefinition, but in the common case where there's no directive, I think it looks cleaner.

leebyron · 2017-12-13T23:22:42Z

Some argument for the other form is that the variable definitions sort of "attach" to the preceding fragment name - because of how we're used to reading myFunction(arg) instead of myFunction (arg) - perhaps it's best for these to look like they parameterize the fragment name instead of the fragment type. Operations like query Name($var: Type) { field } and SDL field definitions like field(arg: Type): Type are other examples of this.

Though I agree with you that when there are multiple variable definitions such that the cleanest rendering is multi-line, then having the type condition no longer on that first line is a real drawback

sam-swarr · 2017-12-13T23:28:01Z

Yeah I can see fair arguments for either. I'm not attached to the current approach, so happy to switch it if you'd like.

dschafer · 2017-12-14T04:57:16Z

I had the same initial reaction as @sam-swarr where "variables first" looked wrong to me because it didn't have the closing parenthesis immediately before the open curly brace. I think my reaction stemmed from my language background: I mostly used languages where return types precede the function (Java or C style) or there is no return type (JS or PHP style), and hence I expect the curly brace right after the close parenthesis.

Looking at languages that declare the return type after the function (like Flow/Hack/Swift/Rust), though:

// Flow
function concat(a: string, b: string): string {
  return a + b;
}

// Hack
public function alpha(): int {
  return 1;
}

// Swift
func greet(person: String, day: String) -> String {
    return "Hello \(person), today is \(day)."
}

It seems like having something between the arguments and the brace is becoming more common... and I do like the fact that in "variables first" the arguments attach to the fragment name, instead of attaching to the type name.

I don't feel strongly either way, though if it were up to me and I had to make the call, I'd lean towards "variables first".

My 2¢.

mjmahone · 2017-12-14T16:21:22Z

Hmmm. Personally I think I've become convinced that, of the two to begin experimenting with, I'd try "variables first" for now. This is explicitly an experimental feature, so if we need to change it because over and over again it "keeps looking wrong" we can with a pretty trivial codemod. So I'm happiest if we pick something, but I suspect @leebyron's instincts are "more right" in that the latter, "variables last" proposal will end up feeling off in more cases compared to "variables first".

sam-swarr · 2017-12-14T18:09:27Z

Having slept on it and after hearing everyone's input, I'm also in the variables first camp now. Updated the commit to reflect that.

dschafer · 2017-12-14T18:18:51Z

src/language/__tests__/parser-test.js

@@ -391,6 +391,16 @@ describe('Parser', () => {
 expect(result.loc).to.equal(undefined);
 });

+ it('Experimental: allows parsing fragment defined variables', () => {


Is it worth adding a test that verifies that without the experimentalFragmentVariables: true flag, this does not parse? Makes sure that we've gated the experimental parsing correctly. (applies to each of the tests, I suspect)

This test case already has that (see my comment below).

dschafer · 2017-12-14T18:20:05Z

Looks great to me!

sam-swarr · 2017-12-14T18:21:23Z

src/language/__tests__/parser-test.js

+ expect(() =>
+ parse(source, { experimentalFragmentVariables: true }),
+ ).to.not.throw();
+ expect(() => parse(source)).to.throw('Syntax Error');


@dschafer, already got it covered here!

dschafer · 2017-12-14T18:22:11Z

src/language/__tests__/parser-test.js

+ expect(() =>
+ parse(source, { experimentalFragmentVariables: true }),
+ ).to.not.throw();
+ expect(() => parse(source)).to.throw('Syntax Error');


Ah, didn't notice this test did both, you're way ahead of me!

IvanGoncharov · 2017-12-14T18:36:36Z

I also have some ideas for a few experiments with GraphQL syntax both on the client-side for query transformations and on the server-side for schema stitching.

Can we wrap all methods from parser.js into class so they call each other only through this? This will allow me to make a derived class that overrides only methods that I need without a need for maintaining my custom fork of graphql-js. It's probably too much implementation details to be exported as part of public API but it can be exported only in parser.js with a warning comment that it's not a part of stable API.

@leebyron Would you be interested in such PR?

leebyron · 2017-12-14T20:47:35Z

@IvanGoncharov we discussed taking that exact approach before sending this PR and while there are merits to building an extendable parser, decided not to pursue it this time. I still like the idea though and would be in favor of trying it out in a PR to see what the complexity cost looks like.

leebyron · 2017-12-14T20:50:41Z

Thanks for your feedback, @dschafer - that was a really valuable additional opinion. I'm happy to see some consensus on the grammar.

This PR looks great, @sam-swarr - thanks for your hard work

facebook-github-bot added the CLA Signed label Dec 13, 2017

mjmahone reviewed Dec 13, 2017

View reviewed changes

leebyron requested changes Dec 13, 2017

View reviewed changes

mjmahone approved these changes Dec 13, 2017

View reviewed changes

Add experimental support for parsing variable definitions in fragments

aa92cba

dschafer reviewed Dec 14, 2017

View reviewed changes

sam-swarr commented Dec 14, 2017

View reviewed changes

dschafer reviewed Dec 14, 2017

View reviewed changes

leebyron approved these changes Dec 14, 2017

View reviewed changes

leebyron merged commit ce0a4b9 into graphql:master Dec 14, 2017

OlegIlyenko mentioned this pull request Dec 15, 2017

Add experimental support for parsing variable definitions in fragments sangria-graphql/sangria#313

Closed

danez mentioned this pull request Feb 16, 2018

Port 0.12.3 changes from graphql-js webonyx/graphql-php#248

Merged

45 tasks

IvanGoncharov mentioned this pull request Jul 17, 2018

Fragment variables: enforce fragment variable validation #1422

Closed

erickreutz mentioned this pull request Jul 19, 2018

Added experimentalFragmentVariables compatibility. apollographql/graphql-tag#167

Merged

3 tasks

IvanGoncharov mentioned this pull request Jul 20, 2018

Parser for Language Services and Editor Tooling #1426

Open

cudr mentioned this pull request Aug 30, 2020

Feature request: support parsing options graphile/crystal#1340

Closed

1 task

daniel-nagy mentioned this pull request Dec 27, 2020

Add Fragment Spread Arguments #2871

Closed

kaiba42 mentioned this pull request Jan 13, 2021

[GraphCache] Undocumented optional argument 'variables' for writeFragment urql-graphql/urql#1305

Closed

mjmahone mentioned this pull request Jun 1, 2021

Enable Fragment Arguments (a form of parameterized fragments) during execution #3152

Closed

IvanGoncharov mentioned this pull request Dec 7, 2021

RFC: Client Controlled Nullability Operators Implementation #3281

Closed

twof mentioned this pull request Dec 8, 2021

[RFC] Client Controlled Nullability experiment implementation w/o execution #3418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add experimental support for parsing variable definitions in fragments #1141

Add experimental support for parsing variable definitions in fragments #1141

sam-swarr commented Dec 13, 2017

mjmahone Dec 13, 2017

leebyron Dec 13, 2017

leebyron left a comment

leebyron Dec 13, 2017

leebyron Dec 13, 2017

leebyron Dec 13, 2017

leebyron Dec 13, 2017

leebyron Dec 13, 2017

sam-swarr Dec 13, 2017

leebyron Dec 13, 2017

leebyron Dec 13, 2017

sam-swarr commented Dec 13, 2017

mjmahone left a comment

mjmahone Dec 13, 2017

mjmahone Dec 13, 2017

sam-swarr commented Dec 13, 2017

leebyron commented Dec 13, 2017

leebyron commented Dec 13, 2017

sam-swarr commented Dec 13, 2017

leebyron commented Dec 13, 2017 •

edited

Loading

sam-swarr commented Dec 13, 2017

dschafer commented Dec 14, 2017

mjmahone commented Dec 14, 2017

sam-swarr commented Dec 14, 2017

dschafer Dec 14, 2017

sam-swarr Dec 14, 2017

dschafer commented Dec 14, 2017

sam-swarr Dec 14, 2017

dschafer Dec 14, 2017

IvanGoncharov commented Dec 14, 2017 •

edited

Loading

leebyron commented Dec 14, 2017

leebyron commented Dec 14, 2017

		`fragment ${name} on ${typeCondition} ` +
		wrap('(', join(variableDefinitions, ', '), ') ') +

Add experimental support for parsing variable definitions in fragments #1141

Add experimental support for parsing variable definitions in fragments #1141

Conversation

sam-swarr commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron left a 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

Choose a reason for hiding this comment

sam-swarr commented Dec 13, 2017

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

sam-swarr commented Dec 13, 2017

leebyron commented Dec 13, 2017

leebyron commented Dec 13, 2017

sam-swarr commented Dec 13, 2017

leebyron commented Dec 13, 2017 • edited Loading

sam-swarr commented Dec 13, 2017

dschafer commented Dec 14, 2017

mjmahone commented Dec 14, 2017

sam-swarr commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschafer commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Dec 14, 2017 • edited Loading

leebyron commented Dec 14, 2017

leebyron commented Dec 14, 2017

leebyron commented Dec 13, 2017 •

edited

Loading

IvanGoncharov commented Dec 14, 2017 •

edited

Loading