/graphql-client

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

Add a graphql_client_web crate for browser usage #174

Open
wants to merge 10 commits into
base: master
from

Conversation

Reviewers

@h-michael h-michael

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@tomhoule @h-michael @theduke
@tomhoule
Member

tomhoule commented Oct 17, 2018 •
edited

What needs to be done before this is mergeable:

  • Merge the PR to remove the lifetime parameter from graphql-query
  • Document it (much) better
  • Refine the errors, provide better causes
  • Make the ClientError enum non-exhaustive
  • Make sure it does not panic
  • Review the dependencies
  • Enable setting custom http headers - and have better defaults there
  • Crucially, test this in CI (we are going to have to run a browser there)
  • Write a better wasm example
@tomhoule

This comment has been minimized.

Sign in to view
Member

tomhoule commented Oct 17, 2018

@h-michael As you can see there is a lot of work left, but if you want to review that would be welcome :)

@tomhoule tomhoule force-pushed the graphql-client-web branch from 49e09ef to 6c05e72 Oct 17, 2018

@h-michael

This comment has been minimized.

Sign in to view
Contributor

h-michael commented Oct 17, 2018

@tomhoule Thanks for mentioning to me!
I'm going to review.

@tomhoule tomhoule force-pushed the graphql-client-web branch from 6c05e72 to b6c286a Oct 18, 2018

@tomhoule

tomhoule reviewed Oct 18, 2018
View changes

graphql_client_web/tests/web.rs Outdated
use wasm_bindgen::JsValue;
use wasm_bindgen_test::*;

use std::panic;

This comment has been minimized.

Sign in to view
Copy link
@tomhoule

tomhoule Oct 18, 2018

Member

can be removed
@h-michael

h-michael reviewed Oct 22, 2018
View changes

graphql_client_web/tests/web.rs

assert_eq!(continent_name, "Europe");
}).map_err(|err| {
panic!("{:?}", err);

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

If panic was called this line, did client response error?
graphql_client_web/src/lib.rs
#[test]
fn client_new() {
Client::new("https://example.com/graphql");
Client::new("/graphql");

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

There is no assert.
What do you expect for this test?

This comment has been minimized.

Sign in to view
Copy link
@tomhoule

tomhoule Nov 13, 2018

Member

It's just to check that it doesn't crash
graphql_client_web/Cargo.toml Outdated
@@ -0,0 +1,35 @@
[package]
name = "graphql_client_web"
version = "0.1.0"

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

You said that all the crates have same version.
Is this crate exception?

This comment has been minimized.

Sign in to view
Copy link
@tomhoule

tomhoule Nov 13, 2018

Member

Good point, I'll change
graphql_client_web/src/lib.rs
/// - (optionally) configure it
/// - use it to perform queries with the [call] method
pub struct Client {
endpoint: String,

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

I think that client can have header attributes too.
graphql_client_web/src/lib.rs
/// - (optionally) configure it
/// - use it to perform queries with the [call] method
pub struct Client {
endpoint: String,

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

You have already said at the description, I think client is able to have header attributes.
@h-michael

h-michael reviewed Oct 22, 2018
View changes

graphql_client_web/src/lib.rs Outdated
///
/// not exhaustive
#[derive(Debug, Fail)]
#[non_exhaustive]

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

You should add #![feature(non_exhaustive)], otherwise I can't build.

This comment has been minimized.

Sign in to view
Copy link
@tomhoule

tomhoule Nov 13, 2018

Member

thanks, done
@h-michael

h-michael reviewed Oct 22, 2018
View changes

Cargo.toml
@@ -1,9 +1,9 @@
[workspace]
members = [
"graphql_client_cli",
"graphql_client_codegen",
"graphql_client",
"graphql_client/examples/example_module",
"graphql_client/examples/github",
"graphql_query_derive",

This comment has been minimized.

Sign in to view
Copy link
@h-michael

h-michael Oct 22, 2018

Contributor

missing "graphql_client_web"

@tomhoule tomhoule referenced this pull request Nov 10, 2018

Open

Write a better wasm example #156

@tomhoule tomhoule force-pushed the graphql-client-web branch from 309a562 to 146be14 Nov 13, 2018

@theduke

This comment has been minimized.

Sign in to view
Member

theduke commented Dec 17, 2018

Out of curiosity, why will this be a separate crate rather than a normal feature?
@tomhoule

This comment has been minimized.

Sign in to view
Member

tomhoule commented Dec 18, 2018

It only works on a specific target (wasm32-unknown-unknown) so it looked like it would be complicated to configure. It also has a very specific setup for tests so it seemed to make sense to make it a separate crate.

If the configuration is straightforward we could reexport graphql-client-web as a feature on graphql-client easily, so I don't really see any big drawback to making it a separate crate. I'm open to other ideas of course :)
@tomhoule

This comment has been minimized.

Sign in to view
Member

tomhoule commented Dec 18, 2018

Also as a more general comment, we want client crates for many different platforms (probably graphql-client-node, graphql-client-async, graphql-client-sync, ...) so I didn't want to clutter the core crate too much.
@tomhoule

This comment has been minimized.

Sign in to view
Member

tomhoule commented Dec 18, 2018

I just realized reexporting from graphql_client wouldn't work because this crate depends on the types from graphql_client. That's a usability problem.
@theduke

This comment has been minimized.

Sign in to view
Member

theduke commented Dec 18, 2018

There is some similiar discussion going on here: seanmonstar/reqwest#288

@tomhoule tomhoule force-pushed the graphql-client-web branch 2 times, most recently from eb94cce to 952d04b Dec 19, 2018

tomhoule added some commits Oct 17, 2018

@tomhoule
Add a graphql_client_web crate for browser usage
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
c183af0
@tomhoule
Make graphql-client-web tests fail with debug message
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
0eb9835
@tomhoule
Make the ClientError enum non-exhaustive
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
0289e8a
@tomhoule
Address review comments
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
ab12bc3
@tomhoule
Test network errors
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
0acb86a
@tomhoule
Implement custom headers in graphql-client-web
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
5001a62
@tomhoule
Clean up all compiler warnings
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
b9168ba
@tomhoule
Apply clippy suggestions on graphql-client-web
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
32f7667

@tomhoule tomhoule force-pushed the graphql-client-web branch 4 times, most recently from a559799 to a13eec1 Dec 19, 2018

@theduke

This comment has been minimized.

Sign in to view
Member

theduke commented Dec 20, 2018

Just wanted to say: I tried out this branch on a wasm project, and it works like a charm. 🎆

@tomhoule tomhoule force-pushed the graphql-client-web branch from a13eec1 to a486484 Dec 20, 2018

tomhoule added some commits Dec 19, 2018

@tomhoule
Enable testing of graphql-client-web in CI
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
446aa42
@tomhoule
Move graphql_client_web to the common cargo workspace
Verified
This commit was signed with a verified signature.
@tomhoule tomhoule Tom Houlé
GPG key ID: 467826F87BCDCB19 Learn about signing commits
Loading status checks…
13fee32

@tomhoule tomhoule force-pushed the graphql-client-web branch from a486484 to 13fee32 Dec 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment