Introduce code generation from .graphql files for making queries #52

Enrico2 · 2020-05-29T20:17:37Z

Josh and Alex want to make the CLI make some GraphQL requests.

We have a plan to use https://github.com/graphql-rust/graphql-client with its code generation features along with a schema that we'll keep up to date as a build phase.

To unblock them, this PR adds 2 example queries and tests to show how this works.

I verified this works locally by using my personal api key and removing the should_panic.

Their existing code saves the api key on the file system after authenticating.
We should therefore create a Client instance that's instantiated with the api key auth creates.

See the documentation for the library we're using and see examples here: https://github.com/graphql-rust/graphql-client/tree/master/graphql_client/tests

queerviolet

Looks good! Couple of small things.

queerviolet · 2020-05-30T21:01:23Z

cli/src/client/mod.rs

+static PROD_GQL_API_URL: &str = "https://engine-graphql.apollographql.com/api/graphql";
+
+pub struct Client {
+    personal_api_key: String,


any reason not to call this api_key?

I was thinking that somewhere else in the code we're going to ask the user to use their personal api key obtained from https://engine.apollographql.com/user-settings but actually this code specifically can take any type of API key so I'll rename.

queerviolet · 2020-05-30T21:10:40Z

cli/src/client/queries.rs

+    Ok(response_body.data.and_then(|data| data.me))
+}
+
+mod tests {


Maybe these would be better as documentation tests? Then we get documentation!

Looks like we could also mark them no_run

Good suggestion! They really are just there for documentation :)

Something is not working as expected with the doctests. I tried moving these over and my first hint that they're not working is that IntelliJ doesn't render them as rust code, but as simple comments. So I tried adding a compilation error in them, and everything still passes.
I went to read some docs and couldn't for the life of me figure out how to do this.
I took this example

/// First line is a short summary describing function. /// /// The next lines present detailed documentation. Code blocks start with /// triple backquotes and have implicit `fn main()` inside /// and `extern crate <cratename>`. Assume we're testing `doccomments` crate: /// /// ``` /// let result = doccomments::add(2, 3); /// assert_eq!(res, 5); /// ``` pub fn add(a: i32, b: i32) -> i32 { a + b }

and changed the last line from result to res in the assert to make it not compile, and everything still passes. I tried putting this block in main.rs too just to make sure.

I then tried putting that add block in a different crate (something I set up locally, the minigrep project from The Book) and IT RENDERS THERE. I seriously am lost on this.

Wow.. Figured this out. So I tried running this:

$ cargo test --doc

And got the response:

error: no library targets found in package `apollo-cli`

So, we have as main.rs but no lib.rs -- I created that file and moved all of the mod ___; from main.rs to lib.rs and NOW DOCTESTS WORK.

🤦

rust-lang/rust#50784

Thinking about this, I'd rather not introduce a lib.rs just yet for the sole purpose of doctests. It forces us to mark some modules as public just that main could use them. So, I think I'll keep the code as is so that tests will compile.

Enrico2 · 2020-05-31T21:48:18Z

No clue how Github is counting lines..

jbaxleyiii · 2020-06-01T16:22:31Z

@Enrico2 I have some thoughts on this that I'll leave today

jbaxleyiii

You and I caught up in our call today but my biggest point of feedback is I'd like the schema to be gitignored to reduce change noise in the repo and to reduce the amount of uneeded information (i.e we only use a very small portion of the schema).

Otherwise, I'm so very excited for this as I'm sure @AlexanderMann and @jsegaran are!

.github/workflows/test.yml

jbaxleyiii · 2020-06-01T19:18:24Z

cli/.graphqlconfig

@@ -0,0 +1,16 @@
+{


Is this used instead of a build.rs script?

Oh sorry, I should have clarified. This is specifically for IntelliJ's graphql plugin integration. It allows for code highlighting when writing queries and some other features.

cli/src/client/mod.rs

jbaxleyiii · 2020-06-01T19:24:28Z

cli/src/client/mod.rs

+            .reqwest
+            .post(&self.uri.to_string())
+            .header("Content-Type", "application/json")
+            .header("apollographql-client-name", "internal-apollo-cli")


Do we want to treat this as a new version of the Apollo CLI or a new product all together?

No preference, whatever you think is best..

🤔 I think it would be cool to keep it the same and watch the stats roll over in Graph Manager!

jbaxleyiii · 2020-06-01T19:25:56Z

cli/src/client/mod.rs

+            .send()?;
+
+        let response_body: Response<Q::ResponseData> = res.json()?;
+        Ok(response_body)


One thing this doesn't do is handle GraphQL errors (i.e. { errors }). What is the type of ResponseData here?

response_body is of type:

pub struct Response<Data> { /// The absent, partial or complete response data. pub data: Option<Data>, /// The top-level errors returned by the server. pub errors: Option<Vec<Error>>, }

ResponseData is codegen'd based on the query, in our case it's a struct with a single service field.

These are queries are here for example purposes, we should definitely error check once we have real queries (and then we can delete these)

cli/src/client/queries.rs

Enrico2 · 2020-06-01T20:16:36Z

@jbaxleyiii

You and I caught up in our call today but my biggest point of feedback is I'd like the schema to be gitignored to reduce change noise in the repo and to reduce the amount of uneeded information (i.e we only use a very small portion of the schema).

Yes, although for this PR to unblock @AlexanderMann and @jsegaran, let's check it in so that they're not blocked on the build script we discussed, once I do that one, I'll remove the schema from the repo and .gitignore it.

Enrico2 · 2020-06-02T18:58:16Z

Since this is not approved yet, I might actually change it a bit if apollographql/rover#55 lands first.

…sts being too slow or panicking.

Enrico2 · 2020-06-02T23:42:37Z

Much better :)

Enrico2 requested review from AlexanderMann, jbaxleyiii, queerviolet and jsegaran May 29, 2020 20:17

Enrico2 force-pushed the ran/gql_codegen branch 2 times, most recently from bef8e85 to 9eb4991 Compare May 29, 2020 21:40

queerviolet suggested changes May 30, 2020

View reviewed changes

Enrico2 requested a review from queerviolet May 31, 2020 21:48

Enrico2 force-pushed the ran/gql_codegen branch from 6957167 to 5c39e92 Compare June 1, 2020 16:32

jbaxleyiii suggested changes Jun 1, 2020

View reviewed changes

Enrico2 requested a review from jbaxleyiii June 1, 2020 20:32

jbaxleyiii approved these changes Jun 2, 2020

View reviewed changes

Enrico2 force-pushed the ran/gql_codegen branch from 5b61227 to 6394e5e Compare June 2, 2020 19:16

Ran Magen added 13 commits June 2, 2020 15:39

Introduce code generation from .graphql files for making queries

e5e833f

ignore tests

1155354

Try to run tarpaulin with threads.

76d1077

1 thread, 10 minutes

9c34e13

rename personal_api_key to api_key

9ba9f4b

remove thread limitation -- doesn't seem related to the problem of te…

5594e62

…sts being too slow or panicking.

GraphQLDocument -> String

66b870b

Disable tarpaulin, run tests everywhere.

aab7ac5

Allow for env variable to override AGM api url.

38e6817

change client name and version headers

135d42e

comment out uploading to codecov.io

ac01965

formatting

d92ef58

Remove schema from git, use the one in .schema/

ceaf0b7

Enrico2 force-pushed the ran/gql_codegen branch from 28775c7 to ceaf0b7 Compare June 2, 2020 22:49

Enrico2 merged commit 282eb11 into master Jun 2, 2020

Enrico2 deleted the ran/gql_codegen branch June 2, 2020 23:43

Introduce code generation from .graphql files for making queries #52

Introduce code generation from .graphql files for making queries #52

Uh oh!

Conversation

Enrico2 commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

queerviolet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Enrico2 commented May 31, 2020

Uh oh!

jbaxleyiii commented Jun 1, 2020

Uh oh!

jbaxleyiii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Enrico2 commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Enrico2 commented Jun 2, 2020

Uh oh!

Enrico2 commented Jun 2, 2020

Uh oh!

Uh oh!

Enrico2 commented May 29, 2020 •

edited

Loading

Enrico2 commented Jun 1, 2020 •

edited

Loading