Skip to content

Implement more of FetchOptions#602

Merged
jstarry merged 8 commits into
yewstack:masterfrom
llebout:more_fetch_options
Feb 13, 2020
Merged

Implement more of FetchOptions#602
jstarry merged 8 commits into
yewstack:masterfrom
llebout:more_fetch_options

Conversation

@llebout
Copy link
Copy Markdown
Contributor

@llebout llebout commented Aug 18, 2019

Fixes #601

Add referrer, referrer_policy and integrity

@llebout llebout force-pushed the more_fetch_options branch 3 times, most recently from 27f1a69 to f765971 Compare August 18, 2019 01:29
@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Aug 18, 2019

Don't merge yet.

@llebout llebout force-pushed the more_fetch_options branch 2 times, most recently from 4d24efb to ade8214 Compare August 18, 2019 01:47
@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Aug 18, 2019

@jstarry @deniskolodin I am meeting issues with manual redirect, it seems that when a redirect happens while manual redirect is enabled, it throws an error but I can't catch it with Yew.

@llebout llebout force-pushed the more_fetch_options branch 5 times, most recently from 33fc675 to c61fdf6 Compare August 25, 2019 23:53
@jstarry
Copy link
Copy Markdown
Member

jstarry commented Sep 23, 2019

@Leo-LB sorry for the delays on this, can you provide some steps on how you have been testing manual redirect? I'd be happy to look into it if you can help me get started :)

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Sep 23, 2019

@Leo-LB sorry for the delays on this, can you provide some steps on how you have been testing manual redirect? I'd be happy to look into it if you can help me get started :)

@jstarry Clone https://github.com/leo-lb/random-imgur-wall and use Redirect::Manual at https://github.com/leo-lb/random-imgur-wall/blob/master/web/src/main.rs#L297

@jstarry jstarry added this to the 0.10.1 milestone Nov 11, 2019
@jstarry
Copy link
Copy Markdown
Member

jstarry commented Dec 13, 2019

No confusion, just lack of time. I'm planning on getting this in before v0.11 🤞

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Dec 13, 2019

I think this PR can be merged as-is, the bug I was being hit by isnt part of the changes introduced by this PR.

Rather, I should open an issue about it. It happens on fetch's redirects but this does not do anything about redirects.

@jstarry
Copy link
Copy Markdown
Member

jstarry commented Dec 13, 2019

@Leo-LB ok, as long as CI passes! Could you please rebase and push?

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Dec 13, 2019

@jstarry Done! It should pass CI!

@hgzimmerman
Copy link
Copy Markdown
Member

I think slapping a #[derive(Debug)] (you might as well do #[derive(Debug, Clone, PartialEq)] as well, because the framework cares about them in other places) on both Referrer and ReferrerPolicy should get it past the CI.

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Dec 13, 2019

@hgzimmerman I followed what was on other structures, CI now passes!

@jstarry
Copy link
Copy Markdown
Member

jstarry commented Dec 13, 2019

I would love if this PR included some tests as well 😉

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Dec 13, 2019

To test this, one would need to include a web server inside the tests. Is there anything that tests code with networking yet?

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Dec 13, 2019

Or we can use: https://httpbin.org/ -- is that acceptable?

Comment thread tests/fetch_service.rs Outdated
println!("{:#?}", resp);
});
let task = FetchService::new().fetch_with_options(request, options, callback);
while task.is_active() {}
Copy link
Copy Markdown
Contributor Author

@llebout llebout Dec 14, 2019

Choose a reason for hiding this comment

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

How can I force the FetchTask to execute? Looping doesnt do it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would think that should execute without the while. I assume you aren't seeing a print in the console because a shim between rust's println and the browser console isn't set up, so by default, it should do nothing (although I may be wrong).

I don't recall how to do that with println, but https://github.com/yewstack/web_logger does this for the logging facilities in the log crate.

I may be entirely off-base though, and it isn't making the request at all.

The plain fetch is used here in this example: https://github.com/yewstack/yew/blob/master/examples/dashboard/src/lib.rs#L117

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay I see, I would think that without the loop, FetchTask would get dropped and immediately canceled.

In the example you linked, the FetchTask is returned and stored then, it does not get dropped.
I do not know how I can do that in the context of a test.
Also, I figured I could maybe use stdweb's support for PromiseFutures (koute/stdweb#103) and convert the fetch "handle" to a future I can await but somehow there's nothing about them in stdweb's released documentation? https://docs.rs/stdweb/0.4.20/stdweb/?search=future https://docs.rs/stdweb/0.4.20/stdweb/?search=promise

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I forgot about that - and the example I provided looks suspicious - that fetch task handle should be preserved in the model in some way.

So yeah, I think looping should be the opportune way to handle that in a test environment - but maybe not with is_active() as I don't think that is set to false when the request completes - only when it is explicitly cancelled.

Maybe the best code would be something along the lines of:

  let mut stop = false;
  let callback = Callback::from(move |resp: Response<Result<String, failure::Error>>| {
     stop = true; // Not sure if the Callback's FnOnce bound will allow this to be satisfied
  });
  let task = FetchService::new().fetch_with_options(request, options, callback);
  while !stop {  }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, it doesnt allow it, unfortunately.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tried with an Rc<RefCell<_>> and it loops indefinitely.

use std::cell::RefCell;
use std::rc::Rc;
#[cfg(feature = "wasm_test")]
use wasm_bindgen_test::{wasm_bindgen_test as test, wasm_bindgen_test_configure};
use yew::callback::Callback;
use yew::format::Nothing;
use yew::services::fetch::{self, FetchOptions, Response};
use yew::services::FetchService;
use yew::services::Task;

#[cfg(feature = "wasm_test")]
wasm_bindgen_test_configure!(run_in_browser);

#[test]
fn fetch_redirect_follow() {
  let request = fetch::Request::get("https://httpbin.org/relative-redirect/1")
    .body(Nothing)
    .unwrap();
  let options = FetchOptions {
    redirect: Some(fetch::Redirect::Follow),
    ..FetchOptions::default()
  };
  let is_done = Rc::new(RefCell::new(false));

  let is_done_callback = is_done.clone();
  let callback = Callback::from(move |resp: Response<Result<String, failure::Error>>| loop {
    match is_done_callback.try_borrow_mut() {
      Ok(mut is_done_callback) => {
        *is_done_callback = true;
        return;
      }
      _ => {}
    }
  });
  let task = FetchService::new().fetch_with_options(request, options, callback);
  loop {
    match is_done.try_borrow() {
      Ok(is_done) if *is_done => return,
      _ => {}
    }
  }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When I monitor traffic with Wireshark the request does happen though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also tried with a full Yew app:

#[cfg(feature = "wasm_test")]
use wasm_bindgen_test::{wasm_bindgen_test as test, wasm_bindgen_test_configure};

use yew::format::Nothing;
use yew::services::fetch::{self, FetchOptions, FetchTask, Response};
use yew::services::FetchService;

#[cfg(feature = "wasm_test")]
wasm_bindgen_test_configure!(run_in_browser);

#[test]
fn fetch_redirect_follow() {
  use yew::{html, Component, ComponentLink, Html, ShouldRender};

  struct Model {
    link: ComponentLink<Self>,
    task: Option<FetchTask>,
  }
  enum Msg {
    TestSuccess,
    TestFail(Response<Result<String, failure::Error>>),
  }
  impl Component for Model {
    type Message = Msg;
    type Properties = ();
    fn create(_: Self::Properties, link: ComponentLink<Self>) -> Self {
      let mut model = Model { link, task: None };

      model.task = Some(
        FetchService::new().fetch_with_options(
          fetch::Request::get("https://httpbin.org/relative-redirect/1")
            .body(Nothing)
            .unwrap(),
          FetchOptions {
            redirect: Some(fetch::Redirect::Follow),
            ..FetchOptions::default()
          },
          model
            .link
            .callback(move |resp: Response<Result<String, failure::Error>>| {
              if resp.status().is_success() {
                Msg::TestSuccess
              } else {
                Msg::TestFail(resp)
              }
            }),
        ),
      );

      model
    }
    fn update(&mut self, msg: Self::Message) -> ShouldRender {
      match msg {
        Msg::TestSuccess => {
          self.task = None;
          false
        }
        Msg::TestFail(resp) => {
          self.task = None;
          panic!("{:?}", resp);
        }
      }
    }
    fn view(&self) -> Html {
      html! {}
    }
  }
  yew::start_app::<Model>();
}

And on test execution it returns:

Running headless tests in Chrome on `http://127.0.0.1:36599/`
Try find `webdriver.json` for configure browser's capabilities:
Not found
driver status: signal: 9                          
driver stdout:
    Starting ChromeDriver 78.0.3904.105 (60e2d8774a8151efa6a00b1f358371b1e0e07ee2-refs/branch-heads/3904@{#877}) on port 36599
    Only local connections are allowed.
    Please protect ports used by ChromeDriver and related test frameworks to prevent access by malicious code.

Error: invalid type: map, expected a string at line 1 column 68
error: test failed, to rerun pass '--test fetch_service'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Evidently channels aren't supported.

I'm going to defer to @jstarry at this point as I don't have experience in writing tests in this context.

Copy link
Copy Markdown
Contributor Author

@llebout llebout Dec 17, 2019

Choose a reason for hiding this comment

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

It looks more supported to write asynchronous tests to test asynchronous functions. https://rustwasm.github.io/docs/wasm-bindgen/wasm-bindgen-test/asynchronous-tests.html

Problem: Yew implements callback based services and not Futures.

Copy link
Copy Markdown
Contributor Author

@llebout llebout left a comment

Choose a reason for hiding this comment

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

I would need some help here

@jstarry jstarry removed this from the 0.11 milestone Jan 6, 2020
@jstarry jstarry added this to the 0.12 milestone Jan 6, 2020
@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Jan 6, 2020

@jstarry Hey! Seems like there is some work to do on making the Yew<->JS interop side more testable as a whole 😅 -- PS: 🎉 on release

@jstarry
Copy link
Copy Markdown
Member

jstarry commented Jan 6, 2020

@jstarry Hey! Seems like there is some work to do on making the Yew<->JS interop side more testable as a whole 😅 -- PS: 🎉 on release

Yeah there really is! Testing has been neglected for awhile 😅 Sorry this didn't make it into the latest release!

@jstarry jstarry force-pushed the more_fetch_options branch from 4128340 to 5eb8a20 Compare January 26, 2020 15:30
@jstarry
Copy link
Copy Markdown
Member

jstarry commented Jan 26, 2020

@Leo-LB today I added some test utils for wrapping a task callback into a future so that we can use async tests. Could you please pull my changes and finish adding tests for referrer-policy and integrity? referrer-policy might not be fully possible since we run tests on a local webdriver.

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Jan 31, 2020

@Leo-LB today I added some test utils for wrapping a task callback into a future so that we can use async tests. Could you please pull my changes and finish adding tests for referrer-policy and integrity? referrer-policy might not be fully possible since we run tests on a local webdriver.

@jstarry
Hey! Thanks. I'll be at FOSDEM, I'll have a look when I'm back!

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Feb 11, 2020

@jstarry

On the tests, I am getting these errors:

    Finished test [unoptimized + debuginfo] target(s) in 0.10s
    Processing "yew-fc6fcbbcc219ca7a.wasm"...
    Finished processing of "yew-fc6fcbbcc219ca7a.wasm"!
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
    Processing "derive_props_test-b8ecff6b83bdffb4.wasm"...
    Finished processing of "derive_props_test-b8ecff6b83bdffb4.wasm"!
    Finished dev [unoptimized + debuginfo] target(s) in 0.10s
    Processing "macro_test-1deabd762309850c.wasm"...
    Finished processing of "macro_test-1deabd762309850c.wasm"!
test result (async): ok. 0 passed; 0 failed

Finished loading Rust wasm module 'yew_fc6fcbbcc219ca7a'
Error loading Rust wasm module 'derive_props_test_b8ecff6b83bdffb4': <RuntimeError>
error: unhandled exception thrown
error:     RuntimeError: unreachable
    at __rust_start_panic (wasm-function[11040]:0x35f234)
    at rust_panic (wasm-function[11026]:0x35eef1)
    at _ZN3std9panicking20rust_panic_with_hook17hdf14da40c6b51ea2E (wasm-function[11021]:0x35ec00)
    at _ZN3std9panicking11begin_panic17h35f8b6795300339eE (wasm-function[10084]:0x3111b3)
    at _ZN8trybuild3run34_$LT$impl$u20$trybuild..Runner$GT$3run28_$u7b$$u7b$closure$u7d$$u7d$17h0775c6174c47cdadE (wasm-function[5126]:0x1c26b3)
    at _ZN4core6result19Result$LT$T$C$E$GT$14unwrap_or_else17hdb569683e1ee4aefE (wasm-function[4063]:0x16869b)
    at _ZN8trybuild3run34_$LT$impl$u20$trybuild..Runner$GT$3run17hfa021fcf2f5ca918E (wasm-function[4302]:0x17d1ce)
    at _ZN61_$LT$trybuild..TestCases$u20$as$u20$core..ops..drop..Drop$GT$4drop17h5a50612b7ff59228E (wasm-function[4310]:0x18517f)
    at _ZN4core3ptr18real_drop_in_place17hda7aa706bd9033ebE (wasm-function[67]:0x5c03)
    at _ZN17derive_props_test5tests17h93b04fd8d7f36f48E (wasm-function[74]:0x62fe)
Error loading Rust wasm module 'macro_test_1deabd762309850c': <RuntimeError>
error: unhandled exception thrown
error:     RuntimeError: unreachable
    at __rust_start_panic (wasm-function[11040]:0x35f35e)
    at rust_panic (wasm-function[11026]:0x35f01b)
    at _ZN3std9panicking20rust_panic_with_hook17hdf14da40c6b51ea2E (wasm-function[11021]:0x35ed2a)
    at _ZN3std9panicking11begin_panic17h35f8b6795300339eE (wasm-function[10084]:0x3112dd)
    at _ZN8trybuild3run34_$LT$impl$u20$trybuild..Runner$GT$3run28_$u7b$$u7b$closure$u7d$$u7d$17h0775c6174c47cdadE (wasm-function[5126]:0x1c27dd)
    at _ZN4core6result19Result$LT$T$C$E$GT$14unwrap_or_else17hdb569683e1ee4aefE (wasm-function[4063]:0x1687c5)
    at _ZN8trybuild3run34_$LT$impl$u20$trybuild..Runner$GT$3run17hfa021fcf2f5ca918E (wasm-function[4302]:0x17d2f8)
    at _ZN61_$LT$trybuild..TestCases$u20$as$u20$core..ops..drop..Drop$GT$4drop17h5a50612b7ff59228E (wasm-function[4310]:0x1852a9)
    at _ZN4core3ptr18real_drop_in_place17hacb694d44bfe438dE (wasm-function[34]:0x4f44)
    at _ZN10macro_test5tests17haec979bec0b2e132E (wasm-function[62]:0x5e7c)

Is that expected?

EDIT:
Nevermind, seems like tests shouldnt be run with cargo web test

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Feb 11, 2020

@jstarry Added. And I tried changing the policy and running the tests in a normal browser by not specifying an headless web driver and it seems that in Chromium, seen in the dev tools Network tab, it's not being changed. I don't know why exactly, if it's Chromium ignoring the option or that the code does not work.

Comment thread src/services/fetch.rs Outdated
@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Feb 11, 2020

@jstarry I made some additional tests with a non-headless browser and I could determine that the referrerPolicy field was being set correctly, without any changes in the code.
Nevermind my previous comment.

Screenshot from 2020-02-11 18-59-35

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Feb 11, 2020

@jstarry I added tests for referrer_policy ; they work if you use NO_HEADLESS=1 cargo test --features wasm_test --target wasm32-unknown-unknown.

@llebout
Copy link
Copy Markdown
Contributor Author

llebout commented Feb 11, 2020

@jstarry
I tried using an headless browser and it produces the Referer header correctly ;

Response {
    status: 200,
    version: HTTP/1.1,
    headers: {
        "content-type": "application/json",
        "content-length": "400",
    },
    body: Json(
        Ok(
            HttpBinHeaders {
                headers: {
                    "X-Amzn-Trace-Id": "Root=1-5e42fb0b-ffdcd0860de6b2b4ce84f3aa",
                    "Origin": "http://127.0.0.1:39793",
                    "Accept-Encoding": "gzip, deflate",
                    "Host": "httpbin.org",
                    "Accept": "*/*",
                    "Referer": "http://127.0.0.1:39793/",
                    "User-Agent": "Mozilla/5.0 (X11; Linux ppc64le) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/78.0.3904.97 Safari/537.36",
                },
            },
        ),
    ),
}

So everything's good!

@jstarry
Copy link
Copy Markdown
Member

jstarry commented Feb 13, 2020

@Leo-LB thanks for all the tests!! This is the start of a new era of Yew 🎉

@jstarry jstarry merged commit 12da2b5 into yewstack:master Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FetchService should support all fetch options

3 participants