This repository has been archived by the owner on Apr 14, 2023. It is now read-only.
Avoid importing graphql/language/printer for getKey. #992
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have been slowly working to remove all uses of the graphql-js print function from Apollo client packages, since the printer weighs in at 4.86KB minified (without gzip); and, while that's not terribly huge, printing large queries takes precious time that we can usually avoid. This particular usage is relatively easy to replace with JSON.stringify. This should be faster than actually printing the query, and we can serialize everything (query, variables, and operation name) with just one call to JSON.stringify, without manually joining the fragments with '|'. It's reasonable to be concerned about the stability of JSON.stringify, since equivalent queries could have slightly different property ordering, but the official GraphQL printer doesn't make any guarantees about the stability or canonicality of the resulting string, either. The important thing is that === identical queries have the same string form, and that's definitely true here. After this change, there is just one remaining call to print in apollo-link-http-common, which seems a bit harder to replace, but you have the option of using something other than apollo-link-http if you really care about not reprinting the query before sending it to the server: https://github.com/apollographql/apollo-link/blob/ed1800b4261566e91906cdb9cc45c7d004393175/packages/apollo-link-http-common/src/index.ts#L239
Codecov Report
@@ Coverage Diff @@
## master #992 +/- ##
=======================================
Coverage 95.19% 95.19%
=======================================
Files 22 22
Lines 1020 1020
Branches 103 103
=======================================
Hits 971 971
Misses 44 44
Partials 5 5
Continue to review full report at Codecov.
|
JoviDeCroock
approved these changes
Mar 18, 2019
Agreed, though I definitely was not expecting that! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have been slowly working to remove all uses of the graphql-js
printfunction from Apollo client packages, since the printer weighs in at 4.86KB minified (without gzip); and, while that's not terribly huge, printing large queries takes precious time that we can usually avoid.This particular usage is relatively easy to replace with
JSON.stringify. This should be faster than actually printing the query, and we can serialize everything (query,variables, andoperationName) with just one call toJSON.stringify, without manually joining the fragments with '|'.It's reasonable to be concerned about the stability of
JSON.stringify, since equivalent queries could have slightly different property ordering, but the official GraphQL printer doesn't make any guarantees about the stability or canonicality of the resulting string, either. The important thing is that===identical queries have the same string form, and that's definitely true here.After this change, there is just one remaining call to
printinapollo-link-http-common, which seems a bit harder to replace, but at least you have the option of using something other than apollo-link-http if you really care about not reprinting the query before sending it to the server.