Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for nested fragments when importing from graphql/loader #227

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var parser = require('graphql/language/parser');

var printer = require('graphql/language/printer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this probably solves the problem thanks the the providing of the much more complete and considerate printer module from graphql-js, introducing this would be adding the full weight of that printer to client bundles, no?

I'm not sure if that was the original reason for using the more simple implementation, but have you evaluated how this change affects bundle size?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't. Unfortunately haven't worked on any GraphQL projects in a while so haven't needed this.
I'll try to find some time over the next week to get back to this

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our team could look at fixing this up so the printer doesn't get included in the client bundle. However, we've never worked on this code base, so have no idea where to start. Can someone provide guidance on an alternative approach that would be accepted? Is there another way we can help get this closer to being merged?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hwillson Looks like you're the most recent contributor to this code base. Is there any hope to get this merged, if we assist? How can we help? Just let us know, thanks :)

var parse = parser.parse;
var print = printer.print;

// Strip insignificant whitespace
// Note that this could do a lot more, such as reorder fields etc.
Expand Down Expand Up @@ -159,7 +160,7 @@ function gql(/* arguments */) {

for (var i = 1; i < args.length; i++) {
if (args[i] && args[i].kind && args[i].kind === 'Document') {
result += args[i].loc.source.body;
result += print(args[i]);
} else {
result += args[i];
}
Expand Down
31 changes: 31 additions & 0 deletions test/graphql.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,37 @@ const assert = require('chai').assert;
assert.equal(definitions[1].kind, 'FragmentDefinition');
});

it('correctly interpolates imports of other files through the webpack loader', () => {
const query = `#import "./fragment_definition.graphql"
fragment BooksAuthor on Book {
author {
...authorDetails
}
}
`;
const jsSource = loader.call({ cacheable() {} }, query);

const oldRequire = require;
const module = { exports: undefined };
const require = (path) => {
assert.equal(path, './fragment_definition.graphql');
return gql`
fragment authorDetails on Author {
firstName
lastName
}`;
};

eval(jsSource);

const document = gql`query { ...BooksAuthor } ${module.exports}`;
assert.equal(document.kind, 'Document');
assert.equal(document.definitions.length, 3);
assert.equal(document.definitions[0].kind, 'OperationDefinition');
assert.equal(document.definitions[1].kind, 'FragmentDefinition');
assert.equal(document.definitions[2].kind, 'FragmentDefinition');
});

it('tracks fragment dependencies across fragments loaded via the webpack loader', () => {
const query = `#import "./fragment_definition.graphql"
fragment F111 on F {
Expand Down