Apollo-client: Fragment already exists warning

[skuridin]

Steps to reproduce

1. Create a named fragment
2. Use it in multiple places

Example of usage

const query = gql`
  query Goal($id: [Int!]) {
    goals(id: $id) {
      ...Goal
    }
  }
  ${Goal.fragments.goal}
`;

Expected

No warnings

Result

Warning that the fragment already exists

Info

[email protected]
├── [email protected] 
├── [email protected] 
└── [email protected] 

[helfer]

@skuridin thanks for reporting this. I don't see apollo client anywhere in your code example, but I'm pretty sure this happens because we currently add all fragments to a dictionary when you run a query. For the time being, if you want to reuse a fragment, you can pass it in with the fragments option.

We're also refactoring how fragments work with apollo client, so I expect this particular oddity to be fixed shortly!

@stubailo is this now the way we recommend that fragments be used??

[skuridin]

@helfer Thanks for the fast response.
Got it.

P.S. React-apollo documentation was updated with the new way of using fragments http://dev.apollodata.com/react/fragments.html

[tmeasday]

@helfer / @stubailo I think we might have crossed wires a little bit here, we should figure out if we want to encourage the above syntax or not. I think we do, now that graphql-tag supports it, it is a bit easier.

For those following along, what @helfer is saying is a workaround is to do

const query = gql`
  query Goal($id: [Int!]) {
    goals(id: $id) {
      ...Goal
    }
  }
`;

// attach the fragment when you use the query, e.g.
@grapqhl(query, {
  fragments: getFragmentDefinitions(Goal.fragments.goal),
});

@helfer should I revert the docs/githunt to using the above syntax for now, or just add a note, or do you think we'll get a fix out soon?

[tmeasday]

OK, slight fix on the above, [Goal.fragments.goal] -> getFragmentDefinitions(Goal.fragments.goal).

(You can import { getFragmentDefinitions } from 'apollo-client')

OTOH, it's probably easier, and has the same effect to just do disableFragmentWarnings() (also imported from apollo-client).

[tmeasday]

(For GitHunt implementation see https://github.com/apollostack/GitHunt-React/commit/44548717ee80d3e60656591519bb3541c9918f65)

[tmeasday]

Actually re-reading https://github.com/apollostack/graphql-tag/issues/18, and looking at the source, I'm not sure this isn't a bug somewhere as the fragment is the same so the warning should skip on this line: https://github.com/apollostack/apollo-client/blob/currentResult-optimisiticResponse/src/fragments.ts#L36

Will investigate further.

[tmeasday]

Hmmm.. Ok. So I guess that check relied on a === which works because gql(X) always returns the same object for the same X. However, even though this test shows the same is true when embedding a fragment (and thus the embedded fragments are the same and the above will work) in the same shaped document, the same property does not hold for embedded fragments within different documents. i.e. this test fails:

    it('returns objects containing the same fragment definition when the same fragment is used in different docs', () => {
      const ast1 = gql`{ foo ...UserFragment } ${fragmentAst}`;
      const ast2 = gql`{ bar ...UserFragment } ${fragmentAst}`;
      assert.deepEqual(ast1.definitions[1], ast2.definitions[1]); // <- this passes
      assert.isTrue(ast1.definitions[1] === ast2.definitions[1]); // <- this does not
    });

@stubailo I guess you are on holiday now but do you think this invalidates our approach of keeping the fragment's src and directly embedding it in the document string, and instead we should be doing something more fancy and working w/ ASTs? (Either that or we should give up on this property of AC).

[helfer]

@tmeasday I'm not sure if there's something efficient we can do. I thought the problem was mostly going to solve itself if people use the fragments in the way we now recommend, because then each fragment should be defined in one and only one place.

Maybe we can downgrade the error to a warning, which encourages people to define fragments in only one place, but still works even if they don't?

[tmeasday]

I thought the problem was mostly going to solve itself if people use the fragments in the way we now recommend, because then each fragment should be defined in one and only one place.

Ultimately it's an issue about assuming that a fragment pulled out of a query document will be the same object as the same fragment pulled out of a different document. At the moment that's not true (see the test above). We could try and make it true, or else actually compare the fragment contents.

Maybe we can downgrade the error to a warning

It already is just a warning.

[dallonf]

Just ran into this - am I understanding correctly that if I have a named fragment _defined_ in one place and _used_ in multiple places (ex: a query and a mutation), it is a bug that this warning appears?

[tmeasday]

Correct.

[tmeasday]

This will be fixed via this PR (and removing fragment warnings from AC) very soon: https://github.com/apollostack/graphql-tag/pull/22

[sedubois]

I just got this warning too with these deps:

    "apollo-client": "^0.5.20",
    "graphql-anywhere": "^1.1.2",
    "graphql-tag": "^1.1.2",
    "react-apollo": "^0.7.1",

Here's my code: https://github.com/relatemind/relate. I get the error when navigating from pages/discover.js to pages/profile.js:

Warning: fragment with name UserPreview already exists.
Warning: fragment with name UserBadge already exists.

[tmeasday]

@sedubois what does the warning you see look like? (i.e. is it coming from AC or GA?).

[sedubois]

@tmeasday it comes from webpack:///./~/apollo-client/fragments.js?2bf5242:29

[tmeasday]

@helfer I think we can safely close this issue and point any further issues at GA, given this functionality is removed from AC, correct?

[tmeasday]

Oh, my bad (thanks for making me look like an idiot github!). I thought we got rid of this warning?

[dallonf]

For what it's worth, the warning disappeared for me with:

[email protected]
[email protected]
[email protected]
[email protected]

These versions are slightly older than @sedubois listed - I wonder if there was a regression?
Guess there's an easy way to find out :) I'm gonna update my dependencies and report back.

Edit:
Updated to:

[email protected]
[email protected]
[email protected]
[email protected]

Still not seeing it. That's... very weird.

Edit: Actually, I am seeing it, just under much more specific conditions that I haven't quite figured out yet. My best guess, based on what I've seen so far and what @sedubois describes, is that it appears when you issue a second query using a particular fragment.

[helfer]

@tmeasday yeah, we haven't removed it yet, but it's slated for 0.6, which I'm planning to release next week with a bunch of new features and an update to the typings.

[calebmer]

0.6 and 0.7 have been released which should include this fix :tada:

Let us know if you are still seeing problems.

[hugofelp]

What's the current way of using fragments with vanilla js?

I tried following this part of the documentation, but it seems to be outdated.

I also tried the solution pointed out by @tmeasday, but that doesn't work for me because I am not using graphql lib directly, and there seems to be no way to reference fragments from a query initiated by an Apollo client.

The only way I managed to get it working, was by injecting fragments as string literals when creating a query. That then will throw a warning from the second time I use a given fragment, saying it already exists.

Can anyone please point me to an example of fragments usage? I am using the latest version 0.10.0 of apollo-client and 1.2.4 of graphql-tag.

[tmeasday]

@hugofelp I'm not sure there's anything in this documentation that is all that react-specific, apart from the fact that you store the fragment on the React component.

I'm not sure what you mean by "I am not using graphql lib directly"?