Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

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

wants to merge 7 commits into from

Conversation

MidasLamb
Copy link

@MidasLamb MidasLamb commented Jul 30, 2020 •
edited
Loading

I've been trying to get @tomhoule code working, but I was wondering why the client was only available for WASM. This PR introduces a general usable client locked after the default feature "http".

@MidasLamb MidasLamb marked this pull request as draft July 30, 2020 19:02
@MidasLamb
Fixed compilation errors and made tests pass
ca0e519 
* Moved tokio (for async tests) to only be used when not targeting wasm.
* Fixed queries for tests to be up to date (ID! instead of String!)
* Fixed example
@MidasLamb MidasLamb changed the title PR To get working on a general client General (optional) Client by using reqwest (as developed by @tomhoule) Jul 30, 2020
@MidasLamb MidasLamb marked this pull request as ready for review July 31, 2020 16:41
@MidasLamb
Copy link
Author

There is still some cleanup to do, but the bulk is done and I think the suggestion this pr is making (including a http client not only for WASM) is ready for discussion

@MidasLamb MidasLamb mentioned this pull request Aug 26, 2020
Closed
@tomhoule tomhoule mentioned this pull request Aug 27, 2020
Open
@MidasLamb
Copy link
Author

@tomhoule, just a friendly reminder, as you said in #327

tomhoule
tomhoule reviewed Sep 30, 2020
View reviewed changes
Copy link
Member

@tomhoule tomhoule left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just started reviewing, and I think we have to work a tiny bit more on it, there's a lot of TODOs that need to be addressed before it goes to the main branch — like you noted in your previous messages Thanks for your effort!

@tomhoule
Copy link
Member

Yes, for the general question: let's have a reqwest feature and keep it as thin a wrapper around vanilla reqwest as possible, on all platforms reqwest supports (that should be automatic, without cfg flagging on the graphql_client side). I also think we can skip testing it, since it's pretty hard to have a real http test setup and we don't have the time resources to maintain it.

@MidasLamb
Removed some excessive left-over code
ae12bdb 
After PR review, removed some excessive leftover comment and debug
statement.
This was referenced Nov 18, 2020
Merged
Merged
@tomhoule
Copy link
Member

This feature is implemented in a simpler way in #371

@tomhoule tomhoule mentioned this pull request Jun 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomhoule tomhoule tomhoule left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MidasLamb @tomhoule