-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
wasm support #10
base: master
Are you sure you want to change the base?
wasm support #10
Conversation
#[cfg(not(target_arch = "wasm32"))] | ||
let client = Client::builder() | ||
.redirect(reqwest::redirect::Policy::none()) | ||
.gzip(true) | ||
.build() | ||
.unwrap(); | ||
#[cfg(target_arch = "wasm32")] | ||
let client = Client::builder().build().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Do you mean gzip
or redirect
is unsupported on wasm32?
redirect
must not be disabled because get_item_download_url
requires it to correctly function. The documentation of new_with_client
already mentioned this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep reqwest
does not have redirect
or gzip
for wasm32
target.
I propose for wasm
we:
- Disable
get_item_download_url
- Add
get_item_content
, which follows redirects and return aVec<u8>
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
reqwest
does not haveredirect
orgzip
forwasm32
target.
Are there target-related difficulties about implementing them? Or is it just reqwest
have not yet implemented them? It seems ClientBuilder
on wasm32 is pretty incomplete (there are even placeholder documentations) and have only few methods available.
- Disable
get_item_download_url
- Add
get_item_content
, which follows redirects and return aVec<u8>
I'm not a fan of providing different (and inconsistent) APIs on different targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
- For
get_item_download_url
, add acfg_if
to panic on wasm with something likenot supported, please use get_item_content
- Implement
get_item_content
for all platforms
I also don't quite get get_item_download_url
. What benefits does it have over simply following redirects and provide get_item_content
directly?
This changes
ResponseExt
andOneDrive::new()
to support wasm32. Also avoids boxing inResponseExt