Stop Bundling GraphQL in the module #46

abhiaiyer91 · 2017-02-01T15:24:05Z

No description provided.

…om graphql. fix maximum callstack issue in stripLoc

stubailo · 2017-02-01T22:39:50Z

index.js

@@ -156,5 +164,7 @@ function gql(/* arguments */) {
 gql.default = gql;
 gql.resetCaches = resetCaches;
 gql.disableFragmentWarnings = disableFragmentWarnings;
+gql.print = require('graphql/language/printer');
+gql.parser = parser


It doesn't look like these two names are consistent - should it be "parse" rather than "parser"?

@stubailo I did a search:
https://github.com/apollographql/apollo-client/search?utf8=%E2%9C%93&q=parser

and saw some typings expecting parser

What do you suggest? parse?

The module is called parser, but the function exported is called parse.

Okay i will export the parse function instead of the whole parser

I was exporting the whole lib var parser = require('graphql/language/parser');

stubailo · 2017-02-01T22:41:06Z

Also it looks like this is adding a yarn lock file, I don't think we want that in the repository.

abhiaiyer91 · 2017-02-01T22:59:35Z

@stubailo I added the lock file because I manage dependencies with yarn. Are we opposed to managing the dependencies and their versions with yarn?

stubailo · 2017-02-01T23:02:47Z

@abhiaiyer91 no, but you can install dependencies with yarn without adding a lock file. Lock files are perfect for applications, but not the right solution for packages, where we need to be constantly testing with new versions.

abhiaiyer91 · 2017-02-02T00:39:39Z

Sounds good, will remove the yarn.lock

abhiaiyer91 · 2017-02-02T15:58:59Z

Alright updates made!

jnwng · 2017-02-07T06:56:08Z

index.js

@@ -90,6 +93,11 @@ function stripLoc (doc, removeLocAtThisLevel) {
    delete doc.loc;
  }

+  if (doc.loc) {


this could use a comment - im not fully certain why we'd want to delete these properties. linking to the issue should be sufficient

Will add a comment, the parser's loc fields include references to the query string and as your AST is deeper it becomes more overhead to ship/store the query

jnwng

this lgtm, but @stubailo should probably sign off.

stubailo · 2017-02-08T00:12:14Z

Yeah I think this is a great change although it is majorly breaking for anyone that was using graphql-tag/parser and graphql-tag/printer. So we should make sure to bump the major version IMO.

Also, are we worried about importing print unconditionally? It's possible that some apps might not be using that, maybe we should just instruct people to import from graphql/language/printer directly?

stubailo · 2017-02-08T00:15:19Z

(Consider that a sign-off, we can always fix it later if we think this increases the bundle size unnecessarily)

abhiaiyer91 · 2017-02-08T00:29:02Z

@stubailo I think people should import printer/parser themselves. They are already going to have graphql due to the peer dependency.

So i would say next steps:

Remove the re-export of both libraries and ship a major version.

abhiaiyer91 · 2017-02-08T00:33:59Z

#49

…rint" This reverts commit f7cf4c2, reversing changes made to c5332e0.

This reverts commit 7413172. # Conflicts: # index.js # parser.js # printer.js # webpack.config.js

PowerKiKi · 2017-04-06T05:55:56Z

Slightly off-topic but yarn.lock file should be committed if you decide to support yarn, as is well explained here.

Would you accept a PR for that ?

jnwng · 2017-04-07T06:00:02Z

thanks for sharing that article, i was fairly certain we shouldn't be adding yarn.lock files. would be happy to accept a PR!

abhiaiyer91 assigned jnwng and stubailo Feb 1, 2017

abhiaiyer91 mentioned this pull request Feb 1, 2017

Stop bundling GraphQL js #6

Closed

abhiaiyer91 added 2 commits February 1, 2017 07:27

fix(bundle): Stop bundling parser and printer ourselves. re-export fr…

54f5432

…om graphql. fix maximum callstack issue in stripLoc

fix(deps): add graphql as a peer dependency

ac061dd

abhiaiyer91 mentioned this pull request Feb 1, 2017

Compiled current version and removed maximum call stack exception #45

Closed

stubailo reviewed Feb 1, 2017

View reviewed changes

abhiaiyer91 added 2 commits February 2, 2017 08:03

fix(lock): remove yarn.lock from source control

72425c4

fix(export): export parse instead of whole parser module

029c39e

jnwng reviewed Feb 7, 2017

View reviewed changes

jnwng approved these changes Feb 7, 2017

View reviewed changes

fix(comments): include a link to the issue surrounding the loc removal

5628384

abhiaiyer91 merged commit f7cf4c2 into master Feb 8, 2017

abhiaiyer91 deleted the ParsePrint branch February 8, 2017 00:34

jnwng mentioned this pull request Feb 18, 2017

Unexpected name "null" when null is provided to an arguments #47

Closed

jnwng added a commit to jnwng/graphql-tag that referenced this pull request Feb 24, 2017

Pulling stripLoc changes from apollographql#46

464cf29

jnwng added a commit to jnwng/graphql-tag that referenced this pull request Feb 28, 2017

Revert "Merge pull request apollographql#46 from apollographql/ParseP…

7413172

…rint" This reverts commit f7cf4c2, reversing changes made to c5332e0.

jnwng added a commit to jnwng/graphql-tag that referenced this pull request Feb 28, 2017

Merge pull request apollographql#46 from apollographql/ParsePrint

f3eea33

This reverts commit 7413172. # Conflicts: # index.js # parser.js # printer.js # webpack.config.js

PowerKiKi mentioned this pull request Apr 7, 2017

Include yarn.lock #72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop Bundling GraphQL in the module #46

Stop Bundling GraphQL in the module #46

abhiaiyer91 commented Feb 1, 2017

stubailo Feb 1, 2017

abhiaiyer91 Feb 1, 2017

stubailo Feb 1, 2017

abhiaiyer91 Feb 2, 2017

abhiaiyer91 Feb 2, 2017

stubailo commented Feb 1, 2017

abhiaiyer91 commented Feb 1, 2017

stubailo commented Feb 1, 2017

abhiaiyer91 commented Feb 2, 2017

abhiaiyer91 commented Feb 2, 2017

jnwng Feb 7, 2017

abhiaiyer91 Feb 7, 2017

jnwng left a comment

stubailo commented Feb 8, 2017

stubailo commented Feb 8, 2017

abhiaiyer91 commented Feb 8, 2017 •

edited

Loading

abhiaiyer91 commented Feb 8, 2017

PowerKiKi commented Apr 6, 2017 •

edited

Loading

jnwng commented Apr 7, 2017

Stop Bundling GraphQL in the module #46

Stop Bundling GraphQL in the module #46

Conversation

abhiaiyer91 commented Feb 1, 2017

stubailo Feb 1, 2017

Choose a reason for hiding this comment

abhiaiyer91 Feb 1, 2017

Choose a reason for hiding this comment

stubailo Feb 1, 2017

Choose a reason for hiding this comment

abhiaiyer91 Feb 2, 2017

Choose a reason for hiding this comment

abhiaiyer91 Feb 2, 2017

Choose a reason for hiding this comment

stubailo commented Feb 1, 2017

abhiaiyer91 commented Feb 1, 2017

stubailo commented Feb 1, 2017

abhiaiyer91 commented Feb 2, 2017

abhiaiyer91 commented Feb 2, 2017

jnwng Feb 7, 2017

Choose a reason for hiding this comment

abhiaiyer91 Feb 7, 2017

Choose a reason for hiding this comment

jnwng left a comment

Choose a reason for hiding this comment

stubailo commented Feb 8, 2017

stubailo commented Feb 8, 2017

abhiaiyer91 commented Feb 8, 2017 • edited Loading

abhiaiyer91 commented Feb 8, 2017

PowerKiKi commented Apr 6, 2017 • edited Loading

jnwng commented Apr 7, 2017

abhiaiyer91 commented Feb 8, 2017 •

edited

Loading

PowerKiKi commented Apr 6, 2017 •

edited

Loading