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

Feature request: strip whitespace from GraphQL queries / fragments / mutations / subscriptions #1523

Closed
rybon opened this issue Sep 14, 2018 · 13 comments
Labels
Milestone

Comments

@rybon
Copy link

@rybon rybon commented Sep 14, 2018

Currently, when issuing requests to a GraphQL API, the GraphQL string describing the query contains a lot of whitespace. While this makes readability in development mode easier, it is wasteful in production.

Would it be possible to add a function to this library that strips this whitespace away? Here's some example code that does this:

let graphQLQuery = `...` // some GraphQL literal

graphQLQuery = graphQLQuery
    .replace(/#.*\n/g, '')
    .replace(/[\s|,]*\n+[\s|,]*/g, ' ')
    .replace(/:\s/g, ':')
    .replace(/,\s/g, ',')
    .replace(/\)\s\{/g, '){')
    .replace(/\}\s/g, '}')
    .replace(/\{\s/g, '{')
    .replace(/\s\}/g, '}')
    .replace(/\s\{/g, '{')
    .replace(/\)\s/g, ')')
    .replace(/\(\s/g, '(')
    .replace(/\s\)/g, ')')
    .replace(/\s\(/g, '(')
    .replace(/=\s/g, '=')
    .replace(/\s=/g, '=')
    .replace(/@\s/g, '@')
    .replace(/\s@/g, '@')
    .replace(/\s\$/g, '$')
    .replace(/\s\./g, '.')
    .trim()

This code turns a GraphQL query such as this one:

query SomeQuery($foo: String!, $bar: String) {
  someField(foo: $foo, bar: $bar) {
    a
    b {
      c
      d
    }
  }
} 

into:

query SomeQuery($foo:String!$bar:String){someField(foo:$foo bar:$bar){a b{c d}}}

Preferably this would happen at compile time for a production build, so the GraphQL literals in the source code would be minified in the output bundle and thus sent in minified form to the GraphQL API.

Proposed name for this function: condense.

@intellix

This comment has been minimized.

Copy link

@intellix intellix commented Sep 14, 2018

We're doing something like this and it transformed a ~800 query into ~400 bytes

@mjmahone

This comment has been minimized.

Copy link
Contributor

@mjmahone mjmahone commented Sep 14, 2018

This library doesn't do any "sending of queries", so I'm not sure what the ask is?

You're welcome to do a pass where you strip whitespace. Probably easier than that would be to gzip your request: that should reduce upload bytes even more than just reducing whitespace.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

@IvanGoncharov IvanGoncharov commented Sep 14, 2018

@mjmahone Minification is different from compression. If you minify source before compression the end result would be smaller than just compression.
For example, for JS we do both using uglify or similar tool and then compress it with gzip.

Plus we have very similar feature request here: #685
That said I think we shouldn't rely on RegExp and have a proper implementation using Lexer class.

@rybon

This comment has been minimized.

Copy link
Author

@rybon rybon commented Sep 14, 2018

It doesn’t do any sending indeed, but it is used by the Relay compiler if I’m not mistaken to extract tagged template literals from the code at compile time.
Maybe I am asking this question in the wrong repository though. If so, where should I ask it instead?

How would one gzip requests on the client? I presume that is not as straightforward as gzipping responses on the server.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

@IvanGoncharov IvanGoncharov commented Sep 14, 2018

Maybe I am asking this question in the wrong repository though. If so, where should I ask it instead?

@rybon Personally, I think it's the right place to implement such functionality since minification is tightly coupled with language grammar so it would make sense to keep it together with lexer and parser.

Plus it would be super simple to implement it on top of Lexer and actual implementation should be pretty small.

@tar1990

This comment was marked as spam.

Copy link

@tar1990 tar1990 commented Sep 18, 2018

💯

@tar1990

This comment was marked as spam.

Copy link

@tar1990 tar1990 commented Sep 18, 2018

👍

@rybon rybon changed the title Feature request: strip whitespace from GraphQL queries / mutations / subscriptions Feature request: strip whitespace from GraphQL queries / fragments / mutations / subscriptions Sep 18, 2018
@IvanGoncharov IvanGoncharov added this to the v14.2.0 milestone Jan 29, 2019
@glasser

This comment has been minimized.

Copy link

@glasser glasser commented Feb 12, 2019

I'd be interested specifically in a normalized whitespace output mode: a serialization of a GraphQL AST whose whitespace (and comments and other ignored tokens) are guaranteed to be deterministic and not change without good reason across releases of graphql-js. Whether that's a reduced mode or not is less relevant; it's just nice to be able to get a deterministic hash of a GraphQL query that is preserved under parsing/printing of the AST.

The Apollo Engine reporting module has a somewhat hacky implementation of this based on post-processing the output of printing, but it would be great to just have this in graphql-js! It looks like #1628 will achieve that goal.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

@IvanGoncharov IvanGoncharov commented Feb 12, 2019

@glasser Thanks for Apollo Engine example.
Does difference in indent inside block strings important in your use case?
For example:

{ test(arg: """
a
""")}
{ test(arg: """
  a
""")}
@glasser

This comment has been minimized.

Copy link

@glasser glasser commented Feb 12, 2019

Great question. Our current implementation normalizes all strings to non-block strings. It looks like the lexer version would not do that, but would normalize to no leading indent? I think either way is fine as well as it's well-determined and unlikely to change accidentally in new graphql-js releases.

@langpavel

This comment has been minimized.

Copy link
Contributor

@langpavel langpavel commented Feb 25, 2019

While this makes readability in development mode easier, it is wasteful in production.

This should not be problem if you use compression on HTTP layer. (edit: there is no compression)
Really. I understand this, but if you really need hyper performance, you may use shared hash key between client and server, something like Automatic Persisted Queries, but you may go far.

Please, don't take me bad, I only wish give good advice, not sink this issue down :-)

@rybon

This comment has been minimized.

Copy link
Author

@rybon rybon commented Feb 26, 2019

@langpavel how do I compress a GraphQL query on the client-side? As far as I know there isn’t an easy way to do that. In Node.js on the server I can just turn on gzip compression and be done with it.

So, the only option left is to use a macro and optimize everything at compile-time. This achieves 2 things:

  1. We get a smaller JS bundle
  2. When running that JS bundle, the GraphQL documents it sends are smaller as well. Yes, I could opt for hashed persisted queries, but that requires more coordination between client and server than I want to get into at the moment

I am a bit amazed that requesting a stripped version of GraphQL document is apparently such a controversial topic. We have been discussing this for a long time. I consider it to be a trivial and obvious feature request, even though the implementation is not.

I appreciate your feedback, but having to wait a good part of a year for this to land is getting hard to justify in my opinion.

@IvanGoncharov

This comment has been minimized.

Copy link
Member

@IvanGoncharov IvanGoncharov commented Feb 26, 2019

I consider it to be a trivial and obvious feature request, even though the implementation is not.

I totally agree with @rybon it's not the responsibility of graphql-js to dictate patterns of the client to server communication instead it should provide common functionality as composable building blocks.

Moreover, this functionality is useful in a number of other cases like storing introspection dumps as minified SDL or having a hash function for graphql documents that doesn't affected by formatting changes.

Please keep this issue on the topic of striping ignored characters and stop discussing best practices for client-server communication.

P.S. Update: I'm working on it ATM with good progress and expect to have it to be ready for review in the next couple days.

IvanGoncharov added a commit to APIs-guru/graphql-js that referenced this issue Mar 26, 2019
Heavily based on work done by @rybon in graphql#1628.
Solves graphql#1523.
IvanGoncharov added a commit to APIs-guru/graphql-js that referenced this issue Mar 26, 2019
Heavily based on work done by @rybon in graphql#1628.
Solves graphql#1523.
IvanGoncharov added a commit to APIs-guru/graphql-js that referenced this issue Mar 26, 2019
Heavily based on work done by @rybon in graphql#1628.
Solves graphql#1523.
IvanGoncharov added a commit to APIs-guru/graphql-js that referenced this issue Mar 26, 2019
Heavily based on work done by @rybon in graphql#1628.
Solves graphql#1523.
IvanGoncharov added a commit to APIs-guru/graphql-js that referenced this issue Mar 26, 2019
Heavily based on work done by @rybon in graphql#1628.
Solves graphql#1523.
IvanGoncharov added a commit that referenced this issue Apr 3, 2019
Heavily based on work done by @rybon in #1628.
Solves #1523.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.