Prepare for release #371

tomhoule · 2021-06-25T07:56:08Z

This is mostly about reducing the amount of moving parts (dependencies, features) so the library is less effort to maintain. Notably, it replaces the custom web client with reqwest. This is in preparation for a 0.10.0 release. The commits should be reviewable separately. We will still need to update the CHANGELOG before release.

Feedback is appreciated :)

closes #338
closes #327

tomhoule · 2021-06-25T07:58:35Z

graphql_client/src/reqwest.rs

+    let reqwest_response = client.post(url).json(&body).send().await?;
+
+    Ok(reqwest_response.json().await?)
+}


This may be too inflexible. Let's wait for feedback.

I'd say so. It's missing error handling at the HTTP level at least. See this code for how GitHub's GQL API is handled.

So basically return the whole response as error in case status != 200? I haven't worked with GraphQL APIs in a long time, but if I remember correctly, APIs weren't consistent on that point. I'll check the graphql spec tomorrow to see if it says anything about it.

I couldn't find anything HTTP-specific in the spec, but there is something on graphql.org. The status code isn't important, the response should always be a GraphQL response body. I'm still going to try to return more information, and fail in a useful way if the response body isn't a valid GraphQL response.

Looks like the reqwest::Error might already give enough context.

(https://docs.rs/reqwest/0.11.4/reqwest/struct.Error.html)

This is more maintenance burden than we can take, and it is better separation of concerns. There was no good third-party wasm web HTTP client when we developed ours, but now there is. Note: the web example compiles, but graphql-hub, the public API we used, is now down.

It is opt-out, but it's a reasonable assumption that it will be there

It is convenient, but not worth adding an extra dependency.

- Remove gitter badge - Removed mention of graphql-client-web - Added mention of the CLI workflow

tomhoule · 2021-06-25T09:15:23Z

@h-michael @mathstuf pinging you just so you notice this, in case you want to review (no pressure though).

graphql_client/src/reqwest.rs

mathstuf · 2021-06-25T17:12:43Z

graphql_client/src/reqwest.rs

+    let reqwest_response = client.post(url).json(&body).send().await?;
+
+    Ok(reqwest_response.json().await?)
+}


I'd say so. It's missing error handling at the HTTP level at least. See this code for how GitHub's GQL API is handled.

mathstuf · 2021-06-25T17:13:56Z

graphql_client_cli/src/introspect_schema.rs

    use std::io::Write;

    let out: Box<dyn Write> = match output {
-        Some(path) => Box::new(::std::fs::File::create(path)?),
+        Some(path) => Box::new(::std::fs::File::create(path).unwrap()),


Not sure I like dropping decent error handling along with anyhow, but I prefer better error types even in executables, so…

The problem of error handling here is mostly about displaying useful context when something fails (nothing is handled). In my experience, anyhow isn't much better at this than panics. I'd be ok reverting the anyhow drop for the CLI. For reference, this is the error type I work with most of the time: https://github.com/prisma/prisma-engines/blob/master/migration-engine/connectors/migration-connector/src/error.rs — it's evolved into its current form over time, and it's exactly the right thing for error reporting in prisma.

Working on a quick error type for graphql_client_cli

mathstuf · 2021-06-25T17:15:02Z

graphql_client_codegen/src/generated_module.rs

 struct OperationNotFound {
    operation_name: String,
 }

+impl Display for OperationNotFound {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str("Could not find an operation named ")?;


Why not write!(f, "Could not… {} …in the query document.", self.operation_name)?

Definitely an unnecessary micro-optimization, but write!() (format macros in generals) expands to more code than it looks like. It can add up to larger binaries and compile times. I just checked on rust-playground:

use std::fmt::Write; fn main() { let mut out = String::new(); write!(out, "{} {} {}", ">", "abcd", "<"); }

expands to

#![feature(prelude_import)] #[prelude_import] use std::prelude::rust_2018::*; #[macro_use] extern crate std; use std::fmt::Write; fn main() { let mut out = String::new(); out.write_fmt(::core::fmt::Arguments::new_v1(&["", " ", " "], &match (&">", &"abcd", &"<") { (arg0, arg1, arg2) => [::core::fmt::ArgumentV1::new(arg0, ::core::fmt::Display::fmt), ::core::fmt::ArgumentV1::new(arg1, ::core::fmt::Display::fmt), ::core::fmt::ArgumentV1::new(arg2, ::core::fmt::Display::fmt)], })); }

graphql_client_codegen/src/lib.rs

tomhoule · 2021-06-25T19:00:02Z

TODO: check why the readme is missing on crates.io

edit: done

tomhoule · 2021-06-25T19:00:11Z

Thanks for the review, will read tomorrow

Co-authored-by: Ben Boeckel <mathstuf@users.noreply.github.com>

Example error: erewhon.tom.~/src/graphql-client/graphql_client_cli λ cargo run generate oi --schema-path=ui Blocking waiting for file lock on build directory Compiling graphql_client_cli v0.9.0 (/home/tom/src/graphql-client/graphql_client_cli) Finished dev [unoptimized + debuginfo] target(s) in 4.78s Running `/home/tom/src/graphql-client/target/debug/graphql-client generate oi --schema-path=ui` Error: Error generating module code: Could not find file with path: ui Hint: file paths in the GraphQLQuery attribute are relative to the project root (location of the Cargo.toml). Example: query_path = "src/my_query.graphql". Location: graphql_client_cli/src/generate.rs:75:24

mathstuf

Things seem better overall, thanks.

Clippy

74af385

tomhoule force-pushed the prepare-for-release branch from 0331c16 to 33f7153 Compare June 25, 2021 07:57

tomhoule commented Jun 25, 2021

View reviewed changes

tomhoule added 5 commits June 25, 2021 10:00

Use the new cargo resolver

4b55901

Simplify CI setup

dfceebb

Improve error handling in graphql-query-derive

60c5d73

Replace rustfmt_nightly feature in CLI with direct calls to rustfmt

7a20ea3

It is opt-out, but it's a reasonable assumption that it will be there

tomhoule force-pushed the prepare-for-release branch from 33f7153 to 7a20ea3 Compare June 25, 2021 08:00

tomhoule added 4 commits June 25, 2021 10:09

Remove thiserror dependency

8bea26a

It is convenient, but not worth adding an extra dependency.

Fix last compiler errors

3d6ba53

Remove one more dependency

1c9fb6d

Implement reqwest-blocking feature and convert examples

366a370

tomhoule mentioned this pull request Jun 25, 2021

General (optional) Client by using reqwest (as developed by @tomhoule) #338

Closed

tomhoule added 2 commits June 25, 2021 10:45

Last small adjustments

b3fd251

Update README and CHANGELOG for 0.10.0

53121eb

- Remove gitter badge - Removed mention of graphql-client-web - Added mention of the CLI workflow

tomhoule force-pushed the prepare-for-release branch from 5b7eee2 to 53121eb Compare June 25, 2021 09:10

tomhoule commented Jun 25, 2021

View reviewed changes

graphql_client/src/reqwest.rs Show resolved Hide resolved

mathstuf requested changes Jun 25, 2021

View reviewed changes

tomhoule and others added 2 commits June 26, 2021 10:13

Apply suggestions from code review

912ba67

Co-authored-by: Ben Boeckel <mathstuf@users.noreply.github.com>

Document cargo features

52bea6a

tomhoule force-pushed the prepare-for-release branch from 0da5448 to 52bea6a Compare June 26, 2021 08:19

mathstuf approved these changes Jun 27, 2021

View reviewed changes

tomhoule merged commit 364bba1 into master Jun 28, 2021

tomhoule deleted the prepare-for-release branch June 28, 2021 19:58

Prepare for release #371

Prepare for release #371

Uh oh!

Conversation

tomhoule commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomhoule Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tomhoule commented Jun 25, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomhoule commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomhoule commented Jun 25, 2021

Uh oh!

mathstuf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomhoule commented Jun 25, 2021 •

edited

Loading

tomhoule Jun 25, 2021 •

edited

Loading

tomhoule commented Jun 25, 2021 •

edited

Loading