Fragment nesting and deduplication

[real34]

I have been facing issues when loading several fragments across files.

Inline works fine however I found the following limitations:

• fragments are not imported recursively
• the same fragment is inlined multiple times, causing issues with Apollo for instance (a fragment name "xxx" has already been defined)

I tried to start working on it to understand how it worked and solved issues as I found them. The current status is available at https://github.com/real34/tmp-babel-plugin-inline-import-graphql-ast ... this is a temporary repository, and I will work on a clean PR when all these issues will be sorted out but I would love some feedbacks about how to solve the current issue I faced.

The implementation in my repository works fine for all the cases available in https://github.com/real34/tmp-babel-plugin-inline-import-graphql-ast/tree/master/tests/fixtures but I now have a problem in my project using webpack.

The problem is that sometimes files are not bundled in the correct order, and the deduplication I implemented leads to queries using a fragment being built before the fragment being "registered" by Apollo.

It makes me wonder whether inlining is really efficient in the end or if there would always be issues? Do you have any thoughts on this or ideas to make it work?

PS: it is the first time I dig into webpack and babel internals, so there may be tricks I am not aware of.

[detrohutt]

Thanks for bringing this up. I had actually meant to address this in 2.0.0 but I forgot. There is deduplication code recently added to the webpack loader this is based off of and that code can probably be used pretty much verbatim. I'll try adding it today and making a beta release for you to try out.

[detrohutt]

@real34 I've published 2.0.1-beta.1 .. You can grab it with yarn add -D babel-plugin-inline-import-graphql-ast@beta

It passed your tests on my machine. Let me know how it works for you. If you'd like to take a look at the code, it's on the beta branch.

Note: For now, I left out the sourceResolver option you added to your repo. If that's something you really need let me know what it's for and I can probably include it here.

[real34]

Thanks for your responsiveness! It looks great, and much more cleaner than my implementation!

The sourceResolver is the configured babel.moduleResolver that could have been configured by the user or a plugin. In my use case, I implemented a custom babel plugin to resolve themes with fallbacks (e.g: importing ./Foo.js from /path/to/base/index.js could resolve to /path/to/theme/Foo.js if the file exists, and fallback to the default /path/to/base/Foo.js otherwise). Theme fallbacks (using babel) is the reason why I would like to use this babel plugin instead of webpack loader in fact.

That said, I still think there is an issue with deduplication in the case I faced initially. I will try (hopefully tomorrow) to put a sample repo so you can reproduce since it is difficult to explain, by then here are further details:

• Given the following files:
  • /path/index.js -> imports ./category and ./product
  • /path/category/index.js -> imports ./CategoryList.gql
  • /path/category/CategoryList.gql -> imports ../product/ProductItemFragment.gql
  • /path/product/index.js -> imports ./ProductList.gql
  • /path/product/ProductList.gql -> imports ./ProductItemFragment.gql
• bundle files with webpack from the /path/index.js entrypoint
• what happens:
  • /path/category/index.js will be transpiled by babel and the CategoryList.gql query AST will be inlined, along with the ProductItemFragment.gql fragment definition
  • /path/product/index.js will be transpiled by babel and the ProductList.gql query AST will be inlined, but not ProductItemFragment.gql fragment definition (because of deduplication)
  • both pages will be bundled in the same file, but executed at different times
• the issue: when browsing on the Products page and issuing the GraphQL ProductList query, an error Unknown fragment ProductItemFragment is thrown because the Category code has never been evaluated and the fragment definition never registered
• expected: all fragments have been defined before executing a query

I hope it is clearer now, and maybe you could think of a solution using a webpack configuration? I stumble upon this because it seems to be unsolvable due to the nature of inlining... a solution I thought would be to group all fragment definitions in one file and use webpack caching to solve this. I wonder if it is not the reason for the this.cacheable() in https://github.com/apollographql/graphql-tag/blob/57a258713acecde5ebef0c5771a975d6446703c7/loader.js#L43 along with the use of doc.loc.source

[detrohutt]

Thanks for explaining. That seems like a reasonable use case so I'll add the sourceResolver option later today.

The sample repo you mentioned will be very useful and as soon as you get it up I'll do my best to find a solution that covers all your use cases.

I had also wondered about the purpose of this.cacheable() in the file you linked to. You may be right.

Thanks for providing those test cases in your temporary repo. They were very helpful in validating the most recent changes. If you wouldn't mind I'd like to modify them to test more complex query/fragment folder structures and use that as the testing solution for this repo since I never updated the tests from the project I forked. I'll gladly credit you in the README.

[detrohutt]

@real34 I also just had an idea that may or may not be useful(I'm pretty sleepy lol).. perhaps I could offer the option of runtime parsing instead of inlining. When the option is enabled, instead of running gql from within the plugin, it would replace the user's import statement with a call to gql(instead of an AST object as it does now). When processing fragments, it would "inline" them into the top-level query and once that query had been fully resolved with all nested fragments, the resulting self-contained query would be placed in the code as the argument to gql to be processed at runtime. What do you think?

[real34]

Of course for the tests! I don't have much time these days (holidays with family) but I initially wanted to do a clean PR with the tests / fixes, so feel free to reuse them!

Regarding runtime parsing, it could be useful but I don't know graphql-tag well enough to see if it would solve the issue or not.

PS: I might not be able to find time for the sample repo tomorrow, so it might be only for Sunday.

[detrohutt]

@real34 I may have fixed the deduplication bug.. it will now cache fragment names only from the current import instead of the current run. I believe this is the desired behavior. It's 2.0.1-beta.2. Grab it and let me know if that works for you.

[real34]

@detrohutt Well spotted! It seems to solve the issue in the sample project I created to reproduce the issue https://github.com/real34/tmp-babel-inline-gql-issue4/commits/master (see commit history: in beta1 it is broken, but works in latest)

I will now try the beta against my bigger codebase and let you know if it is all green too!

[real34]

Good news, it almost works on the bigger codebase :tada: (I manually patched the built file to add the sourceResolver for babel resolvers)

I still see 2 errors of duplicated fragments. It may be related to a remaining issue with recursive fragment imports (fragments importing other fragments). subFragments may not be properly deduplicated.

In my case I have a Layout containing a CmsBlock (in the footer) along with the main Home page content which also contains a CmsBlock. Both CmsBlock components are enhanced with a graphql(CmsBlockQuery) from the following gql:

#import "theme/organisms/CmsBlock/CmsBlockFragment.gql"

query CmsBlockQuery($identifiers: [String]!) {
  cmsBlockList(identifiers: $identifiers) {
    ...CmsBlockFragment
  }
}

The following error is thrown: Error: GraphQL error: There can be only one fragment named "CmsBlockFragment".

I need to leave for now and cannot update the sample repo provided above with this use case yet but still wanted to share the advancements in case you could continue on it.

Other news: I still face the error in #2, and could also help by isolating the issue in a sample repo / test case.

[detrohutt]

It's ok I'm about to go to bed anyway. Thanks again for all your help. With all this info I should be able to squash this bug pretty soon, hopefully later today after I wake up.

[detrohutt]

@real34 I made a change that may help. The deduplication was only being applied to imported fragments, but now it will also dedupe fragments embedded in the top-level query. It was an oversight that needed to be corrected even if it doesn't help in your case. It's 2.0.1-beta.3.

[real34]

@detrohutt Awesome! It seems to have fixed the latest remaining issue related to duplication! Thank you.

May I let you close the issue whenever you feel it's done for you?

[detrohutt]

Yeah I'll close it once a couple more people have used the beta and verified nothing breaks

[xuxucode]

2.0.1-rc.0 works for me :)