-
Notifications
You must be signed in to change notification settings - Fork 575
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
Integrate graphql-crunch #303
Comments
Hi @kachkaev — thanks for your interest. Allow me to shed a little more light on We actually considered using
We had two problems with this behavior:
To fix
We figured that maintaining this invariant would be the safest way to provide a reliable tool, which still accomplishes the same level of compression. In the tweet thread from above, some people raised concerns about how we benchmarked response sizes (uncompressed vs gzip), and whether we benchmarked the uncrunch time. The short answer is: yes, we benchmarked with gzip enabled, and we also included the uncrunch time in our timing benchmarks. The The We focused our benchmarking on React Native clients on iOS and Android. React Native runs on JavaScriptCore, which can be surprisingly slow for (In fact, we even microbenchmarked the performance of pre-allocating a fixed size array for this line of code. I tweeted out the benchmark here.) @stevekrenzel and I think what you've built with |
Thanks for your comment @jamesreggio. I’m totally with you about the performance gains and other benefits of your library and would love it to be a part of graphql-yoga. The only concern I have about I read your source code and was shocked with how small, simple and smart the library is! |
Interesting, @kachkaev the implementation of this would be exactly equal to the one I did for |
It’d be great if TypeScript types we stored directly in @lfades WDYT of supporting |
We can definitely add type definitions. You can track that here: banterfm/graphql-crunch#2 And yes, I definitely recommend the headers and query-param approach. |
I used a header cause I found it more reasonable after reading this issue: gajus/graphql-deduplicator#5, I don't have any real preference there. @jamesreggio I would like to add the types if you don't mind |
I believe that having both GET and headers will be a useful thing. This will work for various scenarios and won’t hurt anyone :–) |
This sounds very interesting! Could you summarize the pros & cons for replacing |
@schickling I don't think there is a need to choose between the two, at least for backwards-compatibility reasons. Both compression libraries are pretty small and can be painlessly included in parallel like so:
I'd wait a week or two for a reply in banterfm/graphql-crunch#1 and can submit a PR to |
Maybe bundling the two capabilities is a bad idea from the get go, think about express.js and how elegant and thin it is. These customizations should be part of the examples, the core should only provide hooks to implement them. |
@schickling — I explained the differences between @orefalo — the whole idea around |
@kachkaev — may I suggest dropping the |
@jamesreggio I don't have a strong opinion here, I'm just suggesting As of now, I still guess it's a right thing to support ¹ Of course, if we don't count that someone would want to DDOS a server a bit more efficiently by turning compression on 😅 In any case, we'll have a feature flag in options to disable |
Just thinking forward, what about adding capabilities around
full stack vs lean frameworks - won't argue, its a matter of opinions. ;-/ |
@orefalo feel free to suggest any of those as a PR! I agree with @jamesreggio that Have a look at |
@jamesreggio I didn't know anything about that deprecation, maybe we should deprecate that option here and only use a query param 🤔. @kachkaev I added some tests for graphql-deduplicator in this PR. The tests for graphql-crunch will be very much the same 💃 |
@lfades we can deprecate |
Gotcha — I didn't realize that My colleague is working on the per-key crunching which will be a breaking change (hopefully the one and only), so I definitely recommend waiting until it's merged. Should be very soon. |
@jamesreggio speaking of breaking changes, WDYT of renaming |
@kachkaev — hah. If we did that, we'd spin out a different repo and have In fact, I'm sure there are plenty of other lossless JSON compression libraries out there that none of us have considered because we wouldn't want to do the legwork to verify that they produce consistently superior results within our domain of concern (GraphQL payloads). |
Hey @jamesreggio! Could you share your plans on refactoring |
@jamesreggio - not to discount anything you have done (because it's very cool) but is there a reason that you did not choose something like Apache Avro? In the big data world this is already a standard protocol for this: https://en.wikipedia.org/wiki/Apache_Avro Here is a nodejs implementation of the spec: https://github.com/mtth/avsc It does need a schema but we should be able to get that from our info object in graphql. This might be a really good fit or at least worth investigating. I have been watching this thread and its progress and just wanted to put in my 2 cents :) |
Hi @abelovic — yes, we didn't use other serialization solutions because they introduce a dramatic complexity overhead that we didn't want or need. Our goal was to create a drop-in, zero-configuration, application-level compression library that is narrowly tailored to the domain. The library that we built satisfies these requirements. It's trivial to adopt, its source takes fewer than three minutes to read, and it benchmarks extremely well (above and beyond the gains from gzipping) for GraphQL responses that have significant subtree overlap. Its implementation even has a beneficial side-effect wherein equivalent subtrees are referentially equal at deserialization time, which certain GraphQL clients (e.g., I'm sure it's possible to map GraphQL schemas to interface definitions in a number of binary exchange formats, but I'm not convinced you'll see meaningful gains beyond what @kachkaev, we haven't had time to focus on deduplicating keys. Short spans of consecutive bytes (for example, JSON object keys) are exactly what gzip and other Huffman-encoded algorithms excel at compressing, so I suspect the marginal gains would be minimal. @stevekrenzel can chime in, though, because I think he implemented a strawman and saw some benchmark regressions. |
Due to inactivity of this issue we have marked it |
Hey 👋, It seems like this issue has been inactive for some time. In need for maintaining clear overview of the issues concerning the latest version of |
Hey, @Urigo from The Guild here! You might know us from projects such as graphql-code-generator, envelop or graphql-tools. For a long time we thought that the Javascript ecosystem is still missing a lightweight cross-platform, but still highly customizable GraphQL Server. In the past the awesome Prisma team took on that great challenge and now we are happy to announce that we are continuing them and just released GraphQL Yoga 2.0 - Build fast, extensible, and batteries-included (Subscriptions, Serverless, File uploads support) GraphQL APIs in Node.js 🚀 We have been working a long time on version 2.0 and have been using it in our clients projects for a few months now and shared a couple of alpha cycles here. Please try Yoga out again, give us feedback and help us spread the word on the new release! |
Just came across this tweet and could not resist from sharing:
I haven't tested graphql-crunch myself, but it appears to be a great tool doing a similar job to graphql-deduplicator, which is a part of graphql-yoga already.
Adding graphql-crunch to Apollo Server and Apollo Client is very similar to how this is done for graphql-deduplicator, according to the README. It can also become graphql-yoga's built-in feature that gets triggered by a flag and thus save gigabytes of bandwidth worldwide 🌍
WDYT of graphql-crunch? I'll be happy to submit a PR that adds it if there is interest 🙂
The text was updated successfully, but these errors were encountered: