Graphql-tag: Fragment definitions within a graphql document ought to be unique.

[jnwng]

This is a rehash of apollo-client#906, introduced again because the new fragment interpolation pattern doesn't go through the addFragmentsToDocument method that we fixed in apollo-client#913 to remove duplicate fragments.

While a recent change in graphql-tag no longer "warns" when duplicate fragments are in a document, we should probably strip the duplicate fragment from the document altogether in either graphql-tag or apollo-client since this will through errors when getting parsed on the server. Optionally we could consider stripping duplicate fragment definitions from the document altogether; although in the aforementioned commit, @tmeasday says

Note that we are simply checking fragment strings for equality, not ensuring that fragment definitions within the larger graphql document have exact equality. We could do the above, but it'd be quite a bit more complicated, and I'm not sure if there's much benefit. AFAICT the main reason for wanting that before was exactly this functionality.

Steps to Reproduce

Interpolating multiple fragments into the same document generates an invalid GraphQL document:

query {
   ...SomeFragment
}
${SomeFragment}
${SomeFragment}

generates a document like

query {
  ...SomeFragment
}
fragment SomeFragment on SomeResource {
  field
}
fragment SomeFragment on SomeResource {
  field
}

which is invalid (and a server may choke on an invalid GraphQL doc).

I filed this here instead of in apollo-client as the related commit I mentioned above asks why there'd be a benefit to de-duplicating fragments within the document, but this happens more often when the same fragment gets interpolated into the same document twice from different places in the query, not necessarily that the fragment itself is already defined in the document.

[tmeasday]

Hi @jnwng, thanks for the report and you are right that this functionality probably does belong in graphql-tag.

The referenced commit wasn't actually considering the issue you've outlined here; it was simply referring to the original "ensure the user doesn't try and define two fragments with the same name" issue.

Probably the simplest approach to deal with this would be to just run over the generated AST and remove any duplicate fragments, so the reproduction/test case would be even simpler:

gql`
query {
  ...SomeFragment
}
fragment SomeFragment on SomeResource {
  field
}
fragment SomeFragment on SomeResource {
  field
}
` === gql`
query {
  ...SomeFragment
}
fragment SomeFragment on SomeResource {
  field
}
`

[tmeasday]

Rather than doing an equality check on the FragmentDefinitions, we would just check the names for equality.

[tmeasday]

Hmm, the tricky part about making the above give exact equality is that cache keys by input doc string and we don't really want to be messing around with that if we can help it. @stubailo do you think it is required that the two input strings above would output the literal same AST document?

[stubailo]

No I don't think that's required. But they should be semanticallly equal probably.

[czert]

This is still an issue when using graphql-tag/loader and the new #import feature. If a query uses two fragments, both of which #import a third fragment, this third fragment will end up duplicated in the resulting document.

I saw that this was mentioned in PR #28. If you can point me in the right direction, I'd be happy to work on deduplication in the webpack loader.

[smitt04]

So is the loader issue being worked on?

[jameswyse]

I ran in to this issue as well and threw together a quick workaround:

// schema/utils.js
const requireFragment = require.context('schema/fragments', true, /\.(graphql|gql)$/);
const requireQuery = require.context('schema/queries', true, /\.(graphql|gql)$/);
const requireMutation = require.context('schema/mutations', true, /\.(graphql|gql)$/);

export function removeDuplicateFragments (doc) {
  const usedFragments = {};

  doc.definitions = doc.definitions.filter(def => {
    const included = def.kind !== 'FragmentDefinition' || !usedFragments[def.name.value];
    if (included) {
      usedFragments[def.name.value] = true;
    }
    return included;
  });

  return doc;
}

export function getQuery (name) {
  const doc = requireQuery(`./${name}.gql`);
  return removeDuplicateFragments(doc);
}

export function getFragment (name) {
  const doc = requireFragment(`./${name}.gql`);
  return removeDuplicateFragments(doc);
}

export function getMutation (name) {
  const doc = requireMutation(`./${name}.gql`);
  return removeDuplicateFragments(doc);
}

With a directory structure like:

schema
├── utils.js
├── fragments
│   ├── foo.gql
│   ├── bar.gql
│   ├── baz.gql
├── mutations
│   ├── createFoo.gql
│   ├── createBar.gql
│   ├── createBaz.gql
├── queries
│   ├── someQuery.gql
│   ├── anotherQuery.gql

I can then do @graphql(getQuery('someQuery')) instead of importing directly. Hope it helps :)