Skip to content
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

Taking into account Apollo fragment interpolation #33

Merged
merged 2 commits into from
Jan 23, 2017

Conversation

jnwng
Copy link
Contributor

@jnwng jnwng commented Dec 7, 2016

This PR is attempting to resolve #29.

In Apollo, fragment interpolation is now valid within the gql TaggedTemplateLiteral, but only outside of top-level structures like query or mutation. In other clients like Lokka and Relay, fragment interpolation actually results in a document substitution, but in Apollo this gets evaluated at runtime, so we only need to validate the location of the fragment interpolation.

We'll do this by evaluating two things:

  1. Making sure that an error isn't thrown when a valid interpolation is found
  2. Throwing an error when an invalid interpolation is found
    In Apollo, fragment interpolation is invalid inside of query structures, so a document like this should fail:
gql`query { someField ${SomeFragment} }`

NOTE: the evaluation strategy for checking to see if we're outside of a structure is naively evaluating the number of open and closing brackets, not whether or not they're matching. this means that

gql`query }{ ${SomeFragment}`

won't throw an error for invalid interpolation (but the graphql parser itself will throw an error on the invalid document structure). this just seemed easier to do.

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change (omitted because I'm not fully sure I should be doing this)
  • Add your name and email to the AUTHORS file (optional) (omitted because this file does not exist)
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion (omitted because it does not change an external API)

Steps to Reproduce

Recently, graphql-fragments was deprecated and its functionality baked into graphql-tag itself, allowing one to interpolate fragments directly into queries themselves, like so:

gql`
  query {
    someField
    ...SomeFragment
  }
  ${SomeFragment}
`

However, eslint-plugin-graphql will report this as a warning / error because interpolation was previously not valid in gql queries for the Apollo client.

@jnwng
Copy link
Contributor Author

jnwng commented Dec 7, 2016

I had one question - this affects the default behavior of the gql tag. In the case where the environment is not specified, this will throw an error for an otherwise valid Apollo-style fragment interpolation. Should I make the check that throws an error in default mode not throw an error to match the Apollo behavior?

@stubailo
Copy link
Contributor

stubailo commented Dec 7, 2016

Yeah let's make the default compatible with 'graphql-tag' in general.

jnwng added 2 commits December 8, 2016 09:26
In Apollo, fragment interpolation is now valid within the `gql` TaggedTemplateLiteral, but only outside of top-level structures like `query` or `mutation`. In other clients like Lokka and Relay, fragment interpolation actuall results in a text substitution, but in Apollo this gets evaluated at runtime, so we only need to validate the location of the fragment interpolation.
Making sure to check the invalid fragment or variable case in other environments, as well as ensuring that the default env works similar to how Apollo does.
@jnwng jnwng force-pushed the jw/apollo-fragments branch from 1eba826 to 3cbddfb Compare December 8, 2016 17:28
@eigenmannmartin
Copy link
Contributor

@stubailo @jnwng What is the state of this pr? Can i help somehow to get it finished? 👼

@msmfsd
Copy link

msmfsd commented Jan 13, 2017

bump

@turadg
Copy link

turadg commented Jan 13, 2017

fwiw, I tried in the #eslint-plugin-graphql channel. Then to work around this I forked, merged jnwng:jw/apollo-fragments, and published a private package.

@jnwng
Copy link
Contributor Author

jnwng commented Jan 23, 2017

@stubailo @tmeasday @helfer anything i can do to help get a review for this PR? happy to add more tests if its not super clear what's happening here.

@helfer
Copy link
Contributor

helfer commented Jan 23, 2017

@jnwng Sorry this slipped under my radar. Looks good to me, I'll merge it and release a new version!

@helfer helfer merged commit af0fd0c into apollographql:master Jan 23, 2017
@helfer
Copy link
Contributor

helfer commented Jan 23, 2017

0.5.0 published. Thank you all for your patience, and special thanks to @jonwong for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for fragment interpolation in Apollo
6 participants