Skip to content
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

Update wasm-bindgen-futures to only support std::future #1695

Closed
alexcrichton opened this issue Aug 2, 2019 · 16 comments · Fixed by #1741
Closed

Update wasm-bindgen-futures to only support std::future #1695

alexcrichton opened this issue Aug 2, 2019 · 16 comments · Fixed by #1741

Comments

@alexcrichton
Copy link
Contributor

In the discussion from our WG meeting yesterday it's high time that we should probably release a new breaking version of the wasm-bindgen-futures crate which drops support for the futures 0.1 crate and instead only supports std::future::Future.

I think the only major work item here is getting the threaded implementation up to par, but that can always happen in a follow-up release!

@yoshuawuyts
Copy link

yoshuawuyts commented Aug 4, 2019

I think the only major work item here is getting the threaded implementation up to par, but that can always happen in a follow-up release!

Strongly in favor of doing the migration to std futures first, and handling the threaded impl in a follow up release. I've been messing around with wasm futures over the last few days, and it would've saved me quite a bit of debugging had std futures support been the default.

edit: heh, maybe I spoke too soon. By "getting the threaded implementation up to par" does that mean solving the "cannot send between threads" restrictions when using await?

@alexcrichton
Copy link
Contributor Author

@yoshuawuyts oh sorry nah this is unrelated to Rust futures and moreso about how we implement things on WebAssembly. The threads here is when the atomics and bulk-memory features are enabled for being compiled to WebAssembly. Those are upcoming features of the WebAssembly target itself and affects how we provide a runtime for executing futures. Today we "lie" that our executor is Send and Sync because you can't have threads, but once WebAssembly threads are a true thing we can no longer lie because it means our own runtime is broken.

The actual transition will be quite simple actually, it's already done for futures 0.1. Just need to port it to 0.3 :)

@ctaggart
Copy link
Contributor

ctaggart commented Aug 8, 2019

To clarify, this work is required to enable the new async/await syntax, correct? What would this example that @Pauan gave, look like with the new async/await syntax:

fn on_click() {
    state.merge(Action::LoadData);
    spawn_local(indexeddb::open("my_db", 1)
        .then(|db| db.start_transaction())
        .then(|(db, trans)| db.object_store("people"))
        .then(|store| store.get(0))
        .then(|record| {
            state.merge(Action::LoadSucceeded {
                name: record.name,
                age: record.age
            });
        }));
}
fn on_click() {
    state.merge(Action::LoadData);
    spawn_local(async {
        let db = await!(indexeddb::open("my_db", 1))?;
        let trans = await!(db.start_transaction())?;
        let store = await!(db.object_store("people"))?;
        let record = await!(store.get(0))?;
        state.merge(Action::LoadSucceeded {
            name: record.name,
            age: record.age
        });
    });
}

@Pauan
Copy link
Contributor

Pauan commented Aug 8, 2019

@ctaggart wasm-bindgen already supports std Future (you just have to enable the futures_0_3 feature).

This issue is about changing it so it's the default, so you no longer have to enable futures_0_3.

As for your example, it would look like this:

fn on_click() {
    state.merge(Action::LoadData);
    spawn_local(unwrap_future(async {
        let db = indexeddb::open("my_db", 1).await?;
        let trans = db.start_transaction().await?;
        let store = db.object_store("people").await?;
        let record = store.get(0).await?;
        state.merge(Action::LoadSucceeded {
            name: record.name,
            age: record.age
        });
        Ok(())
    }));
}

...except the unwrap_future function hasn't been created yet.

@najamelan
Copy link
Contributor

@ctaggart Note that the compatibility layer from the futures library also works perfectly fine in WASM. So you can really use both, with async-await syntax.

@ctaggart
Copy link
Contributor

ctaggart commented Aug 9, 2019

Thanks! Yes, I'm attempting to do a fetch in wasm, but I'm getting confused how to use async/await with it. I enabled the futures_0_3 feature and started translating the fetch example, but run into this error:

156 |             let resp_value = JsFuture::from(window.fetch_with_request(&request)).await?;
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::future::Future` is not implemented for `wasm_bindgen_futures::JsFuture`

Will it eventually be possible to await a JsFuture? How would you translate this part of the example to use async/await?

    let request_promise = window.fetch_with_request(&request);

    let future = JsFuture::from(request_promise)
        .and_then(|resp_value| {
            // `resp_value` is a `Response` object.
            assert!(resp_value.is_instance_of::<Response>());
            let resp: Response = resp_value.dyn_into().unwrap();
            resp.json()
        })
        .and_then(|json_value: Promise| {
            // Convert this other `Promise` into a rust `Future`.
            JsFuture::from(json_value)
        })
        .and_then(|json| {
            // Use serde to parse the JSON into a struct.
            let branch_info: Branch = json.into_serde().unwrap();

            // Send the `Branch` struct back to JS as an `Object`.
            future::ok(JsValue::from_serde(&branch_info).unwrap())
        });

    // Convert this Rust `Future` back into a JS `Promise`.
    future_to_promise(future)

I tried this, but got the error above and many more:

        async {
            let resp_value = JsFuture::from(window.fetch_with_request(&request)).await?;
            let resp: Response = resp_value.dyn_into().unwrap();
            let json = JsFuture::from(resp.json()).await?;
            let branch: Branch = json.into_serde().unwrap();
            future_to_promise(future::ok(JsValue::from_serde(&branch_info).unwrap()))
        }

@najamelan
Copy link
Contributor

@ctaggart AFAICT JsFuture implements at least futures 0.1 Future trait. You might want to use the compat::Future01CompatExt::compat method from the futures 0.3 library to run your use await syntax. You need to use the trait Future01CompatExt for the method to be available.

@Pauan
Copy link
Contributor

Pauan commented Aug 9, 2019

@ctaggart You have to import use wasm_bindgen_futures::futures_0_3::{JsFuture, spawn_local}; in addition to enabling the futures_0_3 feature. Then it will work.

@ctaggart
Copy link
Contributor

ctaggart commented Aug 9, 2019

I ended up getting this to compile, but wasm_bindgen fails with:

error[E0308]: mismatched types
  --> src\lib.rs:76:1
   |
76 | #[wasm_bindgen]
   | ^^^^^^^^^^^^^^^ expected struct `js_sys::Promise`, found opaque type
   |
   = note: expected type `js_sys::Promise`
              found type `impl std::future::Future`
    pub async fn run(&mut self) -> Promise {
        let mut opts = RequestInit::new();
        opts.method("GET");
        opts.mode(RequestMode::Cors);

        let request = Request::new_with_str_and_init(
            "https://api.github.com/repos/rustwasm/wasm-bindgen/branches/master",
            &opts,
        )
        .unwrap();

        request
            .headers()
            .set("Accept", "application/vnd.github.v3+json")
            .unwrap();

        let window = web_sys::window().unwrap();
        
        let request_promise = window.fetch_with_request(&request);
        let resp_value = JsFuture::from(window.fetch_with_request(&request)).await;
        let resp: Response = resp_value.unwrap().dyn_into().unwrap();
        let json = JsFuture::from(resp.json().unwrap()).await.unwrap();
        let branch: Branch = json.into_serde().unwrap();
        wasm_bindgen_futures::future_to_promise(futures::future::ok(JsValue::from_serde(&branch).unwrap()))
    }

May be I need to adjust the uses some more?

use wasm_bindgen::prelude::*;
use js_sys::Promise;
use serde::{Deserialize, Serialize};
use wasm_bindgen::prelude::*;
use wasm_bindgen::JsCast;
use wasm_bindgen_futures::futures_0_3::JsFuture;
use web_sys::{Request, RequestInit, RequestMode, Response};

@najamelan
Copy link
Contributor

najamelan commented Aug 9, 2019

@ctaggart I'm sorry to see you're still struggling. Do you want to call this function from JavaScript? If so I think it shouldn't be an async function. If you want to await inside of it, just wrap the function body in an async block and turn that into a Promise, something like

#[wasm_bindgen]
pub fn run(&mut self) -> Promise {
   let fut = async
   {
      // ... the rest of the code
      JsValue::from_serde(&branch).unwrap()
   }
    wasm_bindgen_futures::future_to_promise(fut)
 }

ps: this kind of question is better suited for the rust user forum than for this GH issue I think.

@Pauan
Copy link
Contributor

Pauan commented Aug 9, 2019

@ctaggart async functions always return a Future, not a Promise. So you need to do something like this:

#[wasm_bindgen]
impl Foo {
    pub fn run(&mut self) -> Promise {
        future_to_promise(self.run_async())
    }
}

impl Foo {
    async fn run_async(&mut self) -> Result<Branch, JsValue> {
        let mut opts = RequestInit::new();
        opts.method("GET");
        opts.mode(RequestMode::Cors);

        let request = Request::new_with_str_and_init(
            "https://api.github.com/repos/rustwasm/wasm-bindgen/branches/master",
            &opts,
        )?;

        request
            .headers()
            .set("Accept", "application/vnd.github.v3+json")?;

        let window = web_sys::window().unwrap();

        let resp_value = JsFuture::from(window.fetch_with_request(&request)).await?;
        let resp: Response = resp_value.dyn_into().unwrap();
        let json = JsFuture::from(resp.json()?).await?;
        let branch: Branch = json.into_serde().unwrap();
        Ok(JsValue::from_serde(&branch).unwrap())
    }
}

In addition, you are using wasm_bindgen_futures::future_to_promise, but you must use wasm_bindgen_futures::futures_0_3::future_to_promise.

When you're using std Futures, you shouldn't be using wasm_bindgen_futures, only wasm_bindgen_futures::futures_0_3.

@alexcrichton
Copy link
Contributor Author

I think that we may want to defer this switch until futures-preview has been published under the futures crate (aka futures 0.3 is available). @yoshuawuyts do you know if there's a rough timeline for when that will happen?

@yoshuawuyts
Copy link

@yoshuawuyts do you know if there's a rough timeline for when that will happen?

futures-preview has been in alpha for over a year, and as far as I can tell there's no clear timeline available. But I haven't attended the last few checkin meetings on this topic, so I may have missed some important updates.

I feel it's likely we'll have a stable out before 1.39, though I'd hope it'd happen sometime in the next 6 weeks when 1.39 goes into beta. Though I personally don't think it's worth waiting on that since the Futures trait is already available on Rust stable, and the whole ecosystem is already migrating.

@alexcrichton
Copy link
Contributor Author

Hm ok thanks for the info, although it's not so much that we're waiting for interfaces or something like that but moreso we have dependencies on at least the channels implemented there and so that code needs to live somewhere...

@Pauan
Copy link
Contributor

Pauan commented Aug 20, 2019

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Aug 28, 2019
This commit defaults all crates in-tree to use `std::future` by default
and none of them support the crates.io `futures` 0.1 crate any more.
This is a breaking change for `wasm-bindgen-futures` and
`wasm-bindgen-test` so they've both received a major version bump to
reflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.

The movement here liberally uses `async`/`await` to remove the need for
using any combinators on the `Future` trait. As a result many of the
crates now rely on a much more recent version of the compiler,
especially to run tests.

The `wasm-bindgen-futures` crate was updated to remove all of its
futures-related dependencies and purely use `std::future`, hopefully
improving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the `RawWaker` business
since there are no other stable APIs in `std::task` for wrapping these.

This commit also adds support for:

    #[wasm_bindgen_test]
    async fn foo() {
        // ...
    }

where previously you needed to pass `(async)` now that's inferred
because it's an `async fn`.

Closes rustwasm#1558
Closes rustwasm#1695
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Aug 28, 2019
This commit defaults all crates in-tree to use `std::future` by default
and none of them support the crates.io `futures` 0.1 crate any more.
This is a breaking change for `wasm-bindgen-futures` and
`wasm-bindgen-test` so they've both received a major version bump to
reflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.

The movement here liberally uses `async`/`await` to remove the need for
using any combinators on the `Future` trait. As a result many of the
crates now rely on a much more recent version of the compiler,
especially to run tests.

The `wasm-bindgen-futures` crate was updated to remove all of its
futures-related dependencies and purely use `std::future`, hopefully
improving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the `RawWaker` business
since there are no other stable APIs in `std::task` for wrapping these.

This commit also adds support for:

    #[wasm_bindgen_test]
    async fn foo() {
        // ...
    }

where previously you needed to pass `(async)` now that's inferred
because it's an `async fn`.

Closes rustwasm#1558
Closes rustwasm#1695
@alexcrichton
Copy link
Contributor Author

I've posted an initial version of this to #1741

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Aug 28, 2019
This commit defaults all crates in-tree to use `std::future` by default
and none of them support the crates.io `futures` 0.1 crate any more.
This is a breaking change for `wasm-bindgen-futures` and
`wasm-bindgen-test` so they've both received a major version bump to
reflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.

The movement here liberally uses `async`/`await` to remove the need for
using any combinators on the `Future` trait. As a result many of the
crates now rely on a much more recent version of the compiler,
especially to run tests.

The `wasm-bindgen-futures` crate was updated to remove all of its
futures-related dependencies and purely use `std::future`, hopefully
improving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the `RawWaker` business
since there are no other stable APIs in `std::task` for wrapping these.

This commit also adds support for:

    #[wasm_bindgen_test]
    async fn foo() {
        // ...
    }

where previously you needed to pass `(async)` now that's inferred
because it's an `async fn`.

Closes rustwasm#1558
Closes rustwasm#1695
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Sep 5, 2019
This commit defaults all crates in-tree to use `std::future` by default
and none of them support the crates.io `futures` 0.1 crate any more.
This is a breaking change for `wasm-bindgen-futures` and
`wasm-bindgen-test` so they've both received a major version bump to
reflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.

The movement here liberally uses `async`/`await` to remove the need for
using any combinators on the `Future` trait. As a result many of the
crates now rely on a much more recent version of the compiler,
especially to run tests.

The `wasm-bindgen-futures` crate was updated to remove all of its
futures-related dependencies and purely use `std::future`, hopefully
improving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the `RawWaker` business
since there are no other stable APIs in `std::task` for wrapping these.

This commit also adds support for:

    #[wasm_bindgen_test]
    async fn foo() {
        // ...
    }

where previously you needed to pass `(async)` now that's inferred
because it's an `async fn`.

Closes rustwasm#1558
Closes rustwasm#1695
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue Sep 5, 2019
This commit defaults all crates in-tree to use `std::future` by default
and none of them support the crates.io `futures` 0.1 crate any more.
This is a breaking change for `wasm-bindgen-futures` and
`wasm-bindgen-test` so they've both received a major version bump to
reflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.

The movement here liberally uses `async`/`await` to remove the need for
using any combinators on the `Future` trait. As a result many of the
crates now rely on a much more recent version of the compiler,
especially to run tests.

The `wasm-bindgen-futures` crate was updated to remove all of its
futures-related dependencies and purely use `std::future`, hopefully
improving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the `RawWaker` business
since there are no other stable APIs in `std::task` for wrapping these.

This commit also adds support for:

    #[wasm_bindgen_test]
    async fn foo() {
        // ...
    }

where previously you needed to pass `(async)` now that's inferred
because it's an `async fn`.

Closes rustwasm#1558
Closes rustwasm#1695
alexcrichton added a commit that referenced this issue Sep 5, 2019
This commit defaults all crates in-tree to use `std::future` by default
and none of them support the crates.io `futures` 0.1 crate any more.
This is a breaking change for `wasm-bindgen-futures` and
`wasm-bindgen-test` so they've both received a major version bump to
reflect the new defaults. Historical versions of these crates should
continue to work if necessary, but they won't receive any more
maintenance after this is merged.

The movement here liberally uses `async`/`await` to remove the need for
using any combinators on the `Future` trait. As a result many of the
crates now rely on a much more recent version of the compiler,
especially to run tests.

The `wasm-bindgen-futures` crate was updated to remove all of its
futures-related dependencies and purely use `std::future`, hopefully
improving its compatibility by not having any version compat
considerations over time. The implementations of the executors here are
relatively simple and only delve slightly into the `RawWaker` business
since there are no other stable APIs in `std::task` for wrapping these.

This commit also adds support for:

    #[wasm_bindgen_test]
    async fn foo() {
        // ...
    }

where previously you needed to pass `(async)` now that's inferred
because it's an `async fn`.

Closes #1558
Closes #1695
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 a pull request may close this issue.

5 participants