devashishdxt / tonic-web-wasm-client

Other
104 stars 28 forks source link

Support `FetchOptions` (#33) #34

Closed isosphere closed 1 year ago

isosphere commented 1 year ago

As detailed in #33, it would be nice to be able to specify or override the options passed to the Web API fetch() request.

This PR implements the concept. I have used it successfully to specify credentials: include in my own project, and I have also run the simple test suite locally with success.

Here's how I used it in my project:

use tonic_web_wasm_client::{Client, FetchOptions};
use web_sys::RequestCredentials;

let options = FetchOptions{ credentials: RequestCredentials::Include, ..Default::default()};        
let mut query_client: AuthenticationServiceClient<Client> = AuthenticationServiceClient::new(Client::new_with_options(BASE_URL.to_owned(), options));

We could reexport some of these required web_sys structs to make things easier for users.

I have not tested any options besides credentials as of this writing.

The default Client::new function will create a FetchOptions instance with FetchOptions::default(). I have implemented a Default handling that makes our default settings clear.

isosphere commented 1 year ago

Latest commit should resolve the cargo fmt failure.

devashishdxt commented 1 year ago

Thanks for the PR and sorry for delay in response. I'll look into it over next few days (weekend).

devashishdxt commented 1 year ago

Closing in favour of #38

isosphere commented 1 year ago

@devashishdxt - I can see you made some improvements in #38, but I have to think that my code was a base or at least a reference for making those changes. Is there a reason there's no mention of my contribution? You could have submitted your modifications to this PR to maintain attribution, even if it was squash-and-merged.

It's unusual to have your PR to be functionally accepted, but only with all credit to you stripped. If you do this as a matter of policy for all contributions, I would have appreciated some notice; i.e.: "contributions to this repository will receive no attribution".

As it is, I feel robbed.

devashishdxt commented 1 year ago

I apologise for the confusion. The main reason for creating new PR was that I didn't want to expose web-sys types in this crate. Instead, I've created wrapper types with better documentation. I should've committed to your branch instead of creating a new one and I'm really sorry for that. I didn't realise that this may hurt your feelings. I'll make sure to keep this in mind in future.

And of course, you're most welcome to create other PRs (dependency upgrades, etc.) if you want to see your name in GitHub contributors.

isosphere commented 1 year ago

The main reason for creating new PR was that I didn't want to expose web-sys types in this crate.

You can submit changes to an existing PR, you don't have to make a new PR to do this.