Ignore duplicate fragments from imports when using webpack loader

[czert]

This fixes #27 for the case of webpack loader.

As imports are handled at runtime, the duplicity checks need to be performed at runtime, too. I added a "unique" function into every graphql module to makes sure no FragmentDefinition is used twice.

[apollo-cla]

@czert: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[jnwng]

@czert thank for the pull request!

while i think the logic here all looks sound, have you considered exporting and re-using the existing fragment processing logic here? it seems like we have the problem of processing / de-duplicating fragments in multiple places, and it'd be nice to be able consolidate them if possible.

[czert]

@jnwng thanks for getting 'round to this!

I think I could use the slightly more robust check from processFragments (if I find a way to "import" it into the built files), but that would have even larger runtime cost, as the check in processFragments also compares the actual source of fragment definitions. I tried to choose the simplest solution instead.

On the other hand, using a more costly but more robust duplicity check will have the advantage of warning the user in case of two different fragments sharing the same name. Come to think of it, that's quite a likely scenario, and silent deduplication is not a good reaction to that situation (which is what my code does, now).

So... yeah, you're right, I'll look into that :)

[jnwng]

just requesting changes per feedback on previous review.

[jnwng]

@czert do let me know if that overhead is too complicated for this particular use case - ideally i'd just like to be able to consolidate the "uniqueness" part since we're doing it in a couple places now

[czert]

@jnwng the overhead is basically running this line for every fragment: https://github.com/apollographql/graphql-tag/blob/master/index.js#L40

Looking at the implementation of cacheKeyFromLoc again, the overhead is probably not something we have to worry about. It seems worth it, considering it will enable us to display a warning for two different fragments with the same name.

[nl0]

@czert @jnwng Hey guys, thank you for your work here. I've stumbled upon this "bug" while trying to use recursive fragments, like this:

# plainFragment.gql
fragment plainFragment on SomeType {
  field1
  filed2
}

# recursiveFragment.gql
#import "./plainFragment.gql"
fragment recursiveFragment on SomeType {
  ...plainFragment
  field3
}

# query.gql
#import "./plainFragment.gql"
#import "./recursiveFragment.gql"
query someQuery {
  stuff1: stuff {
    ...plainFragment
  }
  stuff2: stuff {
    ...recursiveFragment
}

plainFragment gets imported twice and I get an exception about non-uniquely named fragments (I can just import the recursiveFragment as a workaround for now, but it doesn't seem right, it reminds me of old-style sass imports (basically simple inlining) vs proper dependency tree, when every dependency is executed only once).

That said, I think it's not right to warn about duplicate fragments given the use-case I described. One option would be to use deep equality check and warn only if fragments have the same name, but differ in their "contents", tho this check may be kinda costly for the run-time.

So, do you have any ETA on this PR? Can I help with something?

[czert]

@nl0 My branch should already help you with your use case. As it is now, you won't get any warning, the duplicate fragment will be silently stripped.

I don't have any ETA for finishing this PR, though I hope to get back to it soon. Until then, either use my fork (branch loader-deduplicate) or just strip the duplicate fragments manually. Of course, if you'd like to help, feel free to address the requested changes :)

And yes, if I remember correctly, we will only warn in case two different fragments share the same name; turns out the runtime cost is not too high.

[RStankov]

I had similar issue with @nl0 and @czert patch works great.

Are there any plans for this to be merged?

[xiankai]

I had the same problem, only the error message was a bit more cryptic - the duplicate fragment was reported as "unused".

In my case I had a query that imported the same fragment from different locations - identical to @nl0's use case.

Switching to @czert's fork worked wonders!

[josephsiefers]

@czert Thanks for the fork, works great. Really critical feature. Looking forward to this getting merged.

[jnwng]

@czert after spending a fair amount of time attempting to figure out how to merge the run time fragment checker introduced here in the webpack loader with the fragment checker being run by gql itself, i got stuck in a weird rabbit hole that involved me considering some sort of webpack precompiler that can just magically handle all this.

in any case, i've since emerged from that hole (and after discussion with @stubailo), this is a pretty good solution and we'll go ahead and get it merged. thank for you the work (and for the patience!)

[czert]

@jnwng thanks! Yes, that "strange webpack loop" required for code reuse had me confused too.

However, this means that we still don't warn in cases where two different fragments share a name. In that case, the one deeper in the import tree will get silently dropped...

[jnwng]

we could maybe log it to the console (since this is going to happen at runtime in the webpack loader). if comments were available as part of the AST I would even suggest popping a comment in that something got dropped.

in graphql-tag we warn on the duplicate fragment names and give a way to disable them: https://github.com/apollographql/graphql-tag/blob/master/CHANGELOG.md#v110 but im not certain what the equivalent of "disabling" them looks like in the webpack runtime (outside of using some magic global on the window)

[czert]

@jnwng I think it could be a loader option, like import query from 'graphql-tag/loader?disableFragmentWarnings=1!myfile.gql'. This would remove the logging code from the compiled queries...

[jnwng]

that'd be a reasonable solution! would that globally silence fragment warnings or just for that one included document? (i guess we can only do this import by import)

[czert]

I think that my example would silence the warnings for the one file and all fragments the file imports. However, if you include the option in the webpack config file (where you define default loaders by file extension/regex), that would work for all imported files.

Thing is, we'd probably either have to solve the webpack require loop, or accept even more duplicate code.