Allow fragments to be imported by name when using the webpack loader #257
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks good. |
|
why isn't this being merged? |
|
Can we merge this, please? |
|
@gabsprates I wasn't sure how to interpret your comment - are you saying there is an issue with this PR or that this PR would have solved an issue you had? |
|
@jnwng: Friendly ping, is anything missing here? 🙂 |
|
@jnwng: please, what's the status on this PR? |
|
@jnwng A new year, a new ping for this PR |
|
ping, need this |
|
@dobesv if you’re able, please add a couple of tests verifying the behavior you’ve added here, as well as an addition to the docs to let people know how this works. thank you in advance! |
|
here’s an example of the loader being tested to expose the correct names: https://github.com/apollographql/graphql-tag/blob/master/test/graphql.js#L71
…On Tue, Jan 7 2020 at 9:48 PM, < ***@***.*** > wrote:
Are there currently any tests for graphql-tag/loader ? I didn't find any
from a quick look in the test folder.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#257?email_source=notifications&email_token=AAIRCRRGKJHFMFORMV22F6TQ4VSJPA5CNFSM4HKJWIH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEILIIZY#issuecomment-571901031
) , or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAIRCRXKOPWPEI77D6WIIQTQ4VSJPANCNFSM4HKJWIHQ
).
|
e70aa9c to
a3c6fe9
Compare
|
I added tests and updated the README. I can't quite tell why the tests are failing, the tests that fail don't seem related to my changes. |
|
Tests are passing when I run them on my dev machine. Anyone else who if watching this thread who might know why tests are behaving differently on Travis? Maybe a node version issue? |
|
I would love to use such feature. My project relies a lot on fragments and importing those fragments would save us time (also more organized). Some points:
I believe something went wrong with Travis CI @jnwng My Docker setup: Dockerfile docker-compose.yml Output: |
|
Yet another friendly ping :) |
|
Is this just pending due to merge conflicts? |
a3c6fe9 to
8716145
Compare
|
I rebased on master, pushed, and tested again. Looks like the tests are passing now, which is great. @jnwng any chance you would consider merging this PR now? |
|
Any chance this is going in any soon? In my experience importing graphql queries and not being able to mix them with fragments makes the use of |
|
Note that as a workaround you can import a query document that uses the fragment you want to use, and use the |
|
@jnwng - is there anything that I can do to help merge this PR? happy to help if needed. |
|
For those who don't wanna fork and still desire to use this: you can patch diff --git a/node_modules/graphql-tag/README.md b/node_modules/graphql-tag/README.md
index 56eb23b..f69a219 100644
--- a/node_modules/graphql-tag/README.md
+++ b/node_modules/graphql-tag/README.md
@@ -193,6 +193,32 @@ For **create-react-app** < v2, you'll either need to eject or use [react-app-rew
Testing environments that don't support Webpack require additional configuration. For [Jest](https://facebook.github.io/jest/) use [jest-transform-graphql](https://github.com/remind101/jest-transform-graphql).
+#### Support for fragments
+
+With the webpack loader, you can import fragments by name:
+
+In a file called `query.gql`:
+```graphql
+fragment MyFragment1 on MyType1 {
+ ...
+}
+
+fragment MyFragment2 on MyType2 {
+ ...
+}
+```
+
+And in your JavaScript:
+
+```javascript
+import { MyFragment1, MyFragment2 } from 'query.gql'
+```
+
+Note: If your fragment references other fragments, the resulting document will
+have multiple fragments in it. In this case you must still specify the fragment
+name when using the fragment. For example, with `apollo-client` you would specify
+the `fragmentName` option when using the fragment for cache operations.
+
### Warnings
This package will emit a warning if you have multiple fragments of the same name. You can disable this with:
diff --git a/node_modules/graphql-tag/loader.js b/node_modules/graphql-tag/loader.js
index 4ceac4d..5f554e5 100644
--- a/node_modules/graphql-tag/loader.js
+++ b/node_modules/graphql-tag/loader.js
@@ -53,7 +53,7 @@ module.exports = function(source) {
// at compile time, and then uses those at load time to create minimal query documents
// We cannot do the latter at compile time due to how the #import code works.
let operationCount = doc.definitions.reduce(function(accum, op) {
- if (op.kind === "OperationDefinition") {
+ if (op.kind === "OperationDefinition" || op.kind === "FragmentDefinition") {
return accum + 1;
}
@@ -166,7 +166,7 @@ module.exports = function(source) {
`
for (const op of doc.definitions) {
- if (op.kind === "OperationDefinition") {
+ if (op.kind === "OperationDefinition" || op.kind === "FragmentDefinition") {
if (!op.name) {
if (operationCount > 1) {
throw "Query/mutation names are required for a document with multiple definitions";
diff --git a/node_modules/graphql-tag/test/graphql.js b/node_modules/graphql-tag/test/graphql.js
index e77af23..1c4c8f8 100644
--- a/node_modules/graphql-tag/test/graphql.js
+++ b/node_modules/graphql-tag/test/graphql.js
@@ -141,7 +141,18 @@ describe('gql', () => {
assert.equal(Q3[1].name.value, 'F1');
assert.equal(Q3[2].name.value, 'F2');
- });
+ const F1 = module.exports.F1.definitions;
+ const F2 = module.exports.F2.definitions;
+ const F3 = module.exports.F3.definitions;
+
+ assert.equal(F1.length, 1);
+ assert.equal(F1[0].name.value, 'F1');
+ assert.equal(F2.length, 1);
+ assert.equal(F2[0].name.value, 'F2');
+ assert.equal(F3.length, 1);
+ assert.equal(F3[0].name.value, 'F3');
+
+ });
it('tracks fragment dependencies across nested fragments', () => {
const jsSource = loader.call({ cacheable() {} }, `
@@ -179,8 +190,22 @@ describe('gql', () => {
assert.equal(Q1[2].name.value, 'F22');
assert.equal(Q1[3].name.value, 'F11');
- assert.equal(Q2.length, 1);
- });
+ assert.equal(Q2.length, 1);
+
+ const F11 = module.exports.F11.definitions;
+ const F22 = module.exports.F22.definitions;
+ const F33 = module.exports.F33.definitions;
+
+ assert.equal(F11.length, 1);
+ assert.equal(F11[0].name.value, 'F11');
+ assert.equal(F22.length, 2);
+ assert.equal(F22[0].name.value, 'F22');
+ assert.equal(F22[1].name.value, 'F11');
+ assert.equal(F33.length, 3);
+ assert.equal(F33[0].name.value, 'F33');
+ assert.equal(F33[1].name.value, 'F22');
+ assert.equal(F33[2].name.value, 'F11');
+ });
it('correctly imports other files through the webpack loader', () => {
const query = `#import "./fragment_definition.graphql"
|
|
#331 for ref |
|
If you're using yarn 2 it has a built in package patching method you can use instead of patch-package |
webdeb
reviewed
Dec 28, 2020
| @@ -161,12 +161,12 @@ module.exports = function(source) { | |||
|
|
|||
| return newDoc; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
I can imagine, that this line produces a mental dissonance on the apollo team. They don't know exactly why they can't merge this, they just know that they cannot merge it.
@dobesv removing this line will make wonders
|
Are there any plans to merge this, please? |
Any packages that use graphql-tag/loader under the hood will also benefit.
bfe2436 to
48cb13f
Compare
hwillson
approved these changes
Apr 29, 2021
|
Available in |
|
Thank you @hwillson ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes fragments a bit more convenient to use.
Any packages that use
graphql-tag/loaderunder the hood will also benefit.Likely fix for #102 and #251
Note that if the fragment references other fragments (using fragment spread), you will get a document that has multiple fragments in it. In this case you will have to tell apollo which fragment you want using the
fragmentNameoption.