Support for fragment interpolation in Apollo

[jnwng]

Now that we have support for fragment interpolation via graphql-tag (link to relevant deprecation notice in graphql-fragments), we can now do the following in Apollo:

const fragment = gql`
  fragment Foo on SomeResource {
    someField
    ...SomeOtherFragment
  }
  ${SomeOtherFragment}

However, using this syntax gets caught in eslint-plugin-graphql:
, because previously Apollo recommended against interpolation.

Presumably this is okay to re-enable, but we'd want to still guard against other kinds of interpolation. Perhaps we can allow interpolation if it occurs outside any fragment/query/mutation declarations?

[stubailo]

Perhaps we can allow interpolation if it occurs outside any fragment/query/mutation declarations?

I think that's a good call!

[jnwng]

i dove into this last night, and had some trouble figuring out a good solution to this - wanted to share some context in case you all might have ideas on how to address.

in particular, at the time that we're going through our TaggedTemplateLiteral to replace fragment interpolation, we haven't yet "parsed" the document into GraphQL, so we don't know the "structure" of the particular element we're checking. basically, we dont know whether or not the template interpolation is occurring within a query / mutation / fragment structure (yet).

in the relay and lokka clients, fragment definitions themselves are not interpolated, just the "names" of those fragments (i.e., via ...fragmentName). in apollo, we're interpolating the actual definition. so in this case, we could skip the fragment expression replacement altogether, and we'd resolve the issue, which means:

gql`query { someField } ${someFragment}` 

would get parsed as

gql`query { someField }`

of course, this would not guard against the bad case, which is interpolation within the query / mutation / fragment structures themselves (which is pretty important). we'd need to get to the graphql "parse" step though to be able to do that (i think).

couple of solutions that i wanted to spitball:

1. interpolate a "fake" fragment definition
   that would turn this:

gql`query { someField } ${someFragment}` 

into

gql`query { someField } fragment someFragment on SomeResource { someField }`

unfortunately, this will not validate against a provided remote schema, but would get us a little farther in that graphql itself would lint against the "interpolation in invalid structures" case.

2. in apollo, skip the expression replace and define an apollo-only parse step
   this would happen before the graphql parse step, and would deal with checking the valid and invalid interpolation cases before stripping the interpolation entirely and getting the document checked by graphql

3. don't parse anything, guess the structure of the document
   obviously, the weakest solution - count the brackets in the TaggedTemplateLiteral and if this interpolation occurs when there's an open bracket in the document, throw the "invalid interpolation" error, otherwise, you're at the top-level and you're good to go (in apollo).

any thoughts on alternatives?

[tmeasday]

@jnwng I'm not really up to speed with how this package works but in your solution 1., why could we not just insert the actual definition from the fragment? That's how graphql-tag works.

[jnwng]

@tmeasday to provide you some context, what we're evaluating here is the actual javascript AST - not actually executing the code. so at this particular time, we don't actually have the runtime "fragment" definition referenced by the variable being interpolated, just the knowledge that there is a variable being interpolated. here's a visualization of said AST: http://astexplorer.net/#/2UxKgxp2pf (you can click on the template literals and see the corresponding AST on the right).

make sure to select the babel-eslint option like this:
, which will make sure the AST representation is what we're evaluating.

basically, what we're doing is evaluating the javascript AST first, making some graphql-related substitutions to said AST, and subsequently evaluating the GraphQL AST (which is going to parse the actual GraphQL document). its the first part that's giving me some trouble, since in the javascript AST we don't actually know what the structure of the query is going to look like.

[jnwng]

ping @stubailo, i'm sure you had way too many github notifications to follow up on after your vacation :)

[stubailo]

of course, this would not guard against the bad case, which is interpolation within the query / mutation / fragment structures themselves (which is pretty important). we'd need to get to the graphql "parse" step though to be able to do that (i think).

Wouldn't a janky solution of "no interpolation allowed except outside all curly braces" work to identify this case? We already have to do some sketchy stuff for relay interpolation like generate a fake fragment name so it doesn't seem that much worse :P

[jnwng]

deleted my previous comment because i only realized what you meant - i can combine a couple of these solutions to get to the right place. we'll first 1) remove the fragment from the interpolation step, then 2) make sure its outside of the curly braces to negate the bad case i mentioned above (so, combining the first and third solutions). thanks for the insight!

i'll take this if that's okay!

[stubailo]

Yeah that would be awesome!

[msmfsd]

Thanks @jnwng for this fix!