Compact GraphQL query text #3983

tomgasson · 2022-06-21T05:35:42Z

We noticed that some of our query text contains a significant amount of unnecessary whitespace. Unlike server->client requests, client->server requests generally aren't compressed and would require a js compression library to do so.

This PR adds a feature flag to compact the query text. I saw the pre-existing compact mode wasn't wired up to anything, so I've co-opted it.

As an example of the benefit, we have one query which this shrinks from 84kb to 31kb

We're slowly making our way to persisted queries, but this is an immediate improvement and I imagine we're not the only ones who will benefit from smaller network payloads

Happy to make this a config rather than a feature flag, if you don't think this should be the default output

captbaritone · 2022-06-24T16:32:05Z

This seems reasonable to me! I'd love to see some fixture tests which validate this behavior. I think a set of fixture tests similar to the three sets we have here: https://github.com/facebook/relay/tree/main/compiler/crates/graphql-text-printer/tests.

Ideally we could have tests that look like this (this would be the implementation of the mod.rs file:

Parse the fixture file to AST
Convert the AST to IR
Convert the IR to a program
Print the program (use this as the fixture test output, but also...)
Reparse the fixture output to a new AST
Convert the new AST to a new IR
Use our location agnostic IR equality tests to assert that the two IRs are identical

I think there's one issue here: Which is that we don't yet implement the location agnostic equality test for all IR node types. Maybe you could look into how difficult it would be to add the trait for the missing types?

tomgasson · 2022-06-25T10:22:29Z

compiler/crates/graphql-ir/src/node_identifier.rs

@@ -405,6 +409,16 @@ impl LocationAgnosticPartialEq for Argument {
    }
 }

+impl LocationAgnosticPartialEq for Option<&Argument> {


This impl should be redundant from Option<T>, but I couldn't work out the right ref incantation

tomgasson · 2022-06-25T10:24:32Z

compiler/crates/graphql-text-printer/src/print_to_text.rs

+        } else {
+            self.next_line(false)
+        }
+    }


These separator utils are the main change here, they have conditional behaviour in them which avoids the need for self.options.compact checks everywhere

tomgasson · 2022-06-25T10:25:01Z

compiler/crates/graphql-text-printer/src/print_to_text.rs

+
+    fn print_type(&mut self, type_: &TypeReference) -> FmtResult {
+        write!(self.writer, "{}", self.schema.get_type_string(&type_))
+    }


I pulled out a few duplicated prints as I found them

tomgasson · 2022-06-25T10:32:11Z

compiler/crates/graphql-text-printer/tests/compact/fixtures/compact_test.expected

+query Test{aliasOfFoo:username(name:"val1")@customDirective(level:2){emailAddresses,firstName(if:false,unless:false)},...FX}
+
+fragment FX on Query{aliased:username(name:"argVal"){thin:emailAddresses}}


This test captures the same behaviour as GraphQLJava graphql-java/graphql-java#2872

tomgasson · 2022-06-25T10:34:50Z

compiler/Cargo.lock

@@ -1144,9 +1145,9 @@ dependencies = [

 [[package]]
 name = "once_cell"
-version = "1.8.0"
+version = "1.12.0"


Stale from ee99ea4

tomgasson · 2022-07-08T09:13:10Z

.vscode/settings.json

@@ -8,6 +8,9 @@
      "editor.defaultFormatter": "esbenp.prettier-vscode"
    },
    "prettier.enable": true,
+    "rust-analyzer.rustfmt.extraArgs": [
+      "+nightly"
+    ],


Additional to #3959, this helps contributors use the same setup as the github rust lint action

tomgasson · 2022-07-08T09:22:46Z

compiler/Cargo.lock

@@ -175,16 +175,16 @@ dependencies = [

 [[package]]
 name = "clap"
-version = "3.1.18"
+version = "3.2.8"
 source = "registry+https://github.com/rust-lang/crates.io-index"


The noise here is because automatic updates 8e25d86, 3cfc85f, 8935746 etc aren't also updating the lock file.

It would be nice if they were either kept in sync, or have this file ignored

tomgasson · 2022-07-12T03:07:40Z

@captbaritone I implemented those fixture tests. Does this look alright to you?

Wires the pre-existing `compact` printing into a feature flag. Additionally disables indenting when printing in compact form

facebook-github-bot · 2022-09-28T16:47:08Z

@captbaritone has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

captbaritone · 2022-09-28T16:47:23Z

@tomgasson Sorry I lost track of this. I'll work on importing/rebasing now.

rbalicki2 · 2022-09-28T23:06:18Z

Hey @tomgasson — this PR looks great! Thanks for taking it on. I'm in the process of reviewing it now. We'll fix it up internally and land it, thank you for doing this work!! And thank you for adding a bunch of fixtures, that makes it a lot easier to review.

Some questions for you:

Is the space before fragment necessary? e.g. is this legal?

query MyQuery{blah}fragment UserFragment on User{}

Why is the next line not skippable in print_condition? TBH, I'm not really sure what that code is doing. I couldn't figure out how to test that in a fixture. Do you know what is going on in print_condition? If we can't figure it out, keeping it as an unskippable line-break seems right to me.

tomgasson · 2022-09-29T00:05:13Z

Is the space before fragment necessary? e.g. is this legal?

I think that is legal. Looks like an oversight

Why is the next line not skippable in print_condition

I think it probably is. The code to iterate through the conditions' selections should probably share more with the print_selections code, and use item_separator rather than next_line. This was initially separated here

It will be hard to test this one, because I believe a Condition {} is only ever created as a single element

https://github.com/tomgasson/relay/blob/91251aaa04a9749de5d2ce1946974bc395b5ba2b/compiler/crates/graphql-ir/src/build.rs#L1893-L1901

And sometimes combined via a transform, but I can't work out how to trigger that behavior

https://github.com/tomgasson/relay/blob/91251aaa04a9749de5d2ce1946974bc395b5ba2b/compiler/crates/graphql-ir/src/transform.rs#L295-L299

rbalicki2 · 2022-09-29T00:38:52Z

Okay, cool. I guess a future person can figure out what's going on with that condition code, and clean it up. I'll remove the space internally, no need to change the PR. Thanks!

facebook-github-bot added the CLA Signed label Jun 21, 2022

tomgasson changed the title ~~Compact GraphQL query text output~~ Compact GraphQL query text Jun 21, 2022

tomgasson force-pushed the compact branch 3 times, most recently from 0e15af7 to 79bd3d5 Compare June 23, 2022 02:52

tomgasson force-pushed the compact branch 2 times, most recently from 9916c24 to 4f49252 Compare June 25, 2022 10:47

tomgasson commented Jun 25, 2022

View reviewed changes

tomgasson force-pushed the compact branch from 4f49252 to 02007a7 Compare June 25, 2022 10:52

tomgasson force-pushed the compact branch 2 times, most recently from 176f4f8 to 4bf7eb9 Compare July 8, 2022 09:09

tomgasson commented Jul 8, 2022

View reviewed changes

tomgasson force-pushed the compact branch from 4bf7eb9 to 4ff2982 Compare July 14, 2022 02:34

tomgasson force-pushed the compact branch from 4ff2982 to 7e1c833 Compare August 4, 2022 03:37

tomgasson force-pushed the compact branch 2 times, most recently from 0c59dfb to c06b0b5 Compare September 19, 2022 04:42

Compact GraphQL query text output

c313aea

Wires the pre-existing `compact` printing into a feature flag. Additionally disables indenting when printing in compact form

tomgasson force-pushed the compact branch from c06b0b5 to c313aea Compare September 26, 2022 01:26

facebook-github-bot closed this in e80d6ba Sep 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compact GraphQL query text #3983

Compact GraphQL query text #3983

tomgasson commented Jun 21, 2022 •

edited

Loading

captbaritone commented Jun 24, 2022

tomgasson Jun 25, 2022

tomgasson Jun 25, 2022

tomgasson Jun 25, 2022

tomgasson Jun 25, 2022

tomgasson Jun 25, 2022

tomgasson Jul 8, 2022

tomgasson Jul 8, 2022

tomgasson commented Jul 12, 2022

facebook-github-bot commented Sep 28, 2022

captbaritone commented Sep 28, 2022

rbalicki2 commented Sep 28, 2022 •

edited

Loading

tomgasson commented Sep 29, 2022

rbalicki2 commented Sep 29, 2022

		query Test{aliasOfFoo:username(name:"val1")@customDirective(level:2){emailAddresses,firstName(if:false,unless:false)},...FX}

		fragment FX on Query{aliased:username(name:"argVal"){thin:emailAddresses}}

Compact GraphQL query text #3983

Compact GraphQL query text #3983

Conversation

tomgasson commented Jun 21, 2022 • edited Loading

captbaritone commented Jun 24, 2022

tomgasson Jun 25, 2022

Choose a reason for hiding this comment

tomgasson Jun 25, 2022

Choose a reason for hiding this comment

tomgasson Jun 25, 2022

Choose a reason for hiding this comment

tomgasson Jun 25, 2022

Choose a reason for hiding this comment

tomgasson Jun 25, 2022

Choose a reason for hiding this comment

tomgasson Jul 8, 2022

Choose a reason for hiding this comment

tomgasson Jul 8, 2022

Choose a reason for hiding this comment

tomgasson commented Jul 12, 2022

facebook-github-bot commented Sep 28, 2022

captbaritone commented Sep 28, 2022

rbalicki2 commented Sep 28, 2022 • edited Loading

tomgasson commented Sep 29, 2022

rbalicki2 commented Sep 29, 2022

tomgasson commented Jun 21, 2022 •

edited

Loading

rbalicki2 commented Sep 28, 2022 •

edited

Loading