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

Stop bundling GraphQL js #6

Closed
stubailo opened this issue Jul 7, 2016 · 15 comments
Closed

Stop bundling GraphQL js #6

stubailo opened this issue Jul 7, 2016 · 15 comments

Comments

@stubailo
Copy link
Contributor

stubailo commented Jul 7, 2016

Now that the bundling errors have been fixed we should remove the bundled code from this repo again and test that it still works in react native.

@abhiaiyer91
Copy link
Contributor

Do we still wanna do this?

@stubailo
Copy link
Contributor Author

stubailo commented Dec 1, 2016

This is more critical now, because parsing actually changed in 0.8.x to support null as a literal. If you have the time to do this that would be great!

@abhiaiyer91
Copy link
Contributor

Yeah I can def take a look.

A couple concerns are people who used print/parser from gql tag who will now be broken.

So we will need to plan accordingly.

Another idea is to bundle the newer versions of those libs and bundle with webpack2.

@stubailo
Copy link
Contributor Author

stubailo commented Dec 1, 2016

@abhiaiyer91 oh, I was just thinking this could have a dependency (or peer dep) on graphql and re-export from graphql-tag/print and graphql-tag/parse like before?

@abhiaiyer91
Copy link
Contributor

Nice yeah I'll def do that.

Issues I'm running into right now is how the parse function has changed. We're getting caught in Max call stacks in tests when we stripLoc.

Diving into that will let you know

@stubailo
Copy link
Contributor Author

stubailo commented Dec 1, 2016

how the parse function has changed

I seem to recall something has changed about the loc thing but I can't remember what...

@helfer
Copy link
Contributor

helfer commented Dec 13, 2016

Any update @abhiaiyer91 ?

@jnwng
Copy link
Contributor

jnwng commented Jan 31, 2017

ping @abhiaiyer91, any updates here?

@abhiaiyer91
Copy link
Contributor

Sorry totally forgot about this issue. Let me put it on my schedule for today

@stubailo
Copy link
Contributor Author

We actually had to make some changes to graphql-js recently to make it support Rollup for the Angular integration. So now it should be possible for people to do dead code elimination when using this package, if we remove our own custom bundling!

@abhiaiyer91
Copy link
Contributor

Alright so we definitely have an issue stripping the loc field from the AST generated by the graphql parser. The issue here is in the stripLoc function. As we traverse down the AST deleting loc, we hit a maximum call stack error.

Quick question, why do we stripLoc? Just wondering?

@stubailo
Copy link
Contributor Author

stubailo commented Feb 1, 2017

We do that because if you serialize the query to JSON then we end up with a ton of copies of the query string. Maybe the ast structure changed in a newer version?

@KordonDev
Copy link

I had the same problem with stripLoc. I wrote the problem and a solution in #44

@abhiaiyer91
Copy link
Contributor

#46

@KordonDev nice! I realized the same about the new AST structure last night!

@abhiaiyer91
Copy link
Contributor

This is done, but not released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants