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

a kind of GlobalProxy utility and expose global self for universal window/worker usage #1046

Closed
royaltm opened this issue Nov 22, 2018 · 13 comments

Comments

@royaltm
Copy link

royaltm commented Nov 22, 2018

Currently trying to use web_sys::window from the worker context ends up in a uncatchable error as Window name does not exists in a worker context and fails hard in wasm_bindgen generated js code:

export function __widl_instanceof_Window(idx) {
    return getObject(idx) instanceof Window ? 1 : 0;
}

There is however a WorkerGlobalScope and similar structs, but they share a lot of "global" functions with the Window, like fetch or createImageBitmapWithImageData.

The workaround solution would be to create a GlobalProxy enum with all those common utility methods and then implement it for each of the *GlobalScope structs.

Also exposing the following self function would be very helpfull in this context, with a signature like this:

fn self_() -> Option<GlobalProxy>;

Currently my workaround solution looks like this:

enum GlobalProxy {
    Window(Window),
    WorkerGlobalScope(WorkerGlobalScope),
    //... more scopes
}

impl GlobalProxy {
    fn create_image_bitmap_with_image_data(&self, a_image: &ImageData) -> Result<js_sys::Promise, JsValue> {
        match self {
            GlobalProxy::Window(window) => window.create_image_bitmap_with_image_data(a_image),
            GlobalProxy::WorkerGlobalScope(scope) => scope.create_image_bitmap_with_image_data(a_image),
            //... more of that
        }
        
    }
}

fn self_() -> Result<GlobalProxy, JsValue> {
    let global = js_sys::global();
    // how to properly detect this in wasm_bindgen?
    if js_sys::eval("typeof WorkerGlobalScope !== 'undefined'")?.as_bool().unwrap() {
        Ok(global.dyn_into::<WorkerGlobalScope>().map(GlobalProxy::WorkerGlobalScope)?)
    }
    else {
        Ok(global.dyn_into::<Window>().map(GlobalProxy::Window)?)
    }
}

so i could:

        let scope = self_().unwrap();
        scope.create_image_bitmap_with_image_data(&image_data)

which works regardless if run in a window or a worker context.
However this is very verbose and would be great if such an auto-generated solution would be provided in web-sys. This would be very tedious for an external crate to be in-sync with web-sys.

There is a mixin webidl: https://github.com/rustwasm/wasm-bindgen/blob/master/crates/web-sys/webidls/enabled/WindowOrWorkerGlobalScope.webidl which could be used to auto-generate common global methods of GlobalProxy.

@xtuc
Copy link
Member

xtuc commented Nov 24, 2018

I agree, all generated JS should use access self or global for the global scope and as you mentionned a user-side of getting that reference.

@alexcrichton
Copy link
Contributor

Thanks for the report! I think this is definitely a case where we don't handle this super-well today. One option is to use unchecked_into and friends to sort of "lie" about the types here and get it all working, but that's not the best solution I think. Using an enum may be best done though as a crate which depends on wasm-bindgen and not in wasm-bindgen itself?

It seems reasonable though to at the very least have documentation about this pattern if not a crate which supports it!

@royaltm
Copy link
Author

royaltm commented Nov 26, 2018

but how to do the check: typeof Foo == 'undefined' without eval? I couldn't find any implementation of typeof in the js_sys crate. Is it not supported? If you try to dyn_into some global struct and the struct's name doesn't exist in the javascript scope, bindgen's js code throws an error which can't be intercepted from rust.
At least I'd like to have a way to implement the check without resorting to eval.

@alexcrichton
Copy link
Contributor

We should probably fixup the typeof JS shims to handle when the type isn't defined perhaps? That way the method could just unconditionally return false for types that don't actually exist.

@Pauan
Copy link
Contributor

Pauan commented Nov 26, 2018

@alexcrichton I'm not sure I like that idea. It seems like a footgun to conflate "this isn't the right type" with "this type doesn't exist".

I can see situations where people would want to distinguish those situations (e.g. polyfills), and it can be confusing if the type check silently returns false on some browsers (because the type doesn't exist).

In addition, type detection often involves much more than just typeof, e.g. Modernizr does a lot of various checks to make sure that the actual behavior is correct (not just that the variable exists).

So my feeling is that we should have support for a more general type detection system. Such as an attribute that allows users to provide custom behavior for JsCast::instanceof.


As for the issue about Window/WorkerGlobalScope, it's unfortunate that they don't share a common ancestor, instead they use mixins.

I wonder if web-sys should auto-generate a trait for each mixin, and then impl the trait for each type that uses that mixin.

So in this case it would generate a WindowOrWorkerGlobalScope trait and then impl that for Worker and WorkerGlobalScope. This would be in addition to the current inherent methods + Deref (the trait would just delegate to the inherent methods):

pub trait WindowOrWorkerGlobalScope {
    fn atob(&self, input: &str) -> Result<String, JsValue>;
    ...
}

impl WindowOrWorkerGlobalScope for Window {
    #[inline]
    fn atob(&self, input: &str) -> Result<String, JsValue> {
        // Uses Window::atob
        Self::atob(input)
    }

    ...
}

impl WindowOrWorkerGlobalScope for WorkerGlobalScope {
    #[inline]
    fn atob(&self, input: &str) -> Result<String, JsValue> {
        // Uses WorkerGlobalScope::atob
        Self::atob(input)
    }

    ...
}

I just now tested it, and it seems to work perfectly: using traits works well even when using Deref for inheritance.

So we can use traits for mixins, and Deref for class inheritance, and as far as I can tell everything Just Works: we only need to impl the trait on the root type and all sub-types will get access to it automatically (thanks to Deref).

@Pauan
Copy link
Contributor

Pauan commented Nov 27, 2018

To be clear: my idea is not a rehash of the trait RFC. I have no intention of resurrecting that.

My idea is that we keep everything the same as it is now (inherent methods + Deref), but we just add traits.

Specifically, we add traits only for WebIDL mixins, not for class inheritance. These traits would just delegate to the inherent methods (which already exist today).

This allows for Rust code to work with any type (or sub-type, thanks to Deref) which has that mixin.

Most users don't need to use the traits: they continue to use the inherent methods + Deref, just like today.

But in specific situations like this (where you want the same code on both workers and the window), it is useful to use the trait.

@royaltm
Copy link
Author

royaltm commented Nov 27, 2018

@Pauan your examples only include passing around such Global trait. Not exactly a solution. In practice I would need a function that produces it (that's why I've used a enum). How would you write a self_() function with your traits? E.g. I have a function exposed with #[wasm_bindgen] to JS which creates and returns a bitmap image. Passing to this function a global as an argument isn't the way to go. Perhaps I don't quite understand what you did propose exactly.

@royaltm
Copy link
Author

royaltm commented Nov 27, 2018

@alexcrichton if dyn_into would fail with the proper result in both cases:

  1. the global function (name) does not exist - typeof Name === "undefined"
  2. the variable is not an instance of a global function - getObject(idx) instanceof Name

would be enough for starters. @Pauan IMHO by default you should not care to distinguish those situations while using dyn_into.
If I actually cared to properly detect if I have a Name in scope I would use a different method to do that. E.g. use eval as in my example, or js_sys should provide an equivalent to javascript's typeof.

@alexcrichton
Copy link
Contributor

I think that traits like you mention @Pauan are definitely possible, but I think that it may be a bit too early for such an implementation/addition to web-sys.

I'd prefer if we got a bit more experience using the existing APIs (fixing bugs where necessary) before adding more abstractions. It seems like we should definitely fix the bug where instance_of is throwing right now due to undefined names and go from there?

@Pauan
Copy link
Contributor

Pauan commented Nov 29, 2018

@royaltm In practice I would need a function that produces it (that's why I've used a enum). How would you write a self_() function with your traits?

You would write a function that returns impl WindowOrWorkerGlobalScope (or Box<dyn WindowOrWorkerGlobalScope>).

I haven't thought super hard about this, though, so there might be some limitations I'm not understanding.


@alexcrichton I don't think it's quite right to call it a bug: as a user I would expect it to error, since that is standard behavior in both JavaScript and Rust.

I also don't think that it's good to just return false in the case where a type doesn't exist: that's important information which is being lost.

Thinking about it more, perhaps the best idea is to add a new try_instance_of API, which would catch any errors and return Result<bool, JsValue>.

@royaltm
Copy link
Author

royaltm commented Nov 29, 2018

@Pauan regarding instanceof check "error": currently this is the kind of "error" you can't do anything about it from rust code (it's not even like a panic, it's more like an "abort" in the native code), so yes this is obviously a bug and should be addressed as such. Here's an example: https://github.com/royaltm/rust-wasm-bindgen-window-or-worker-playground/blob/master/tests/wasm.rs#L19

As for returning impl Trait: it only works, when you want to hide your implementation and always return the same type.

The role of self_() is that it may return different things (without recompilation) depending on your context.

The following would not compile (if uncommented): https://github.com/royaltm/rust-wasm-bindgen-window-or-worker-playground/blob/master/src/lib.rs#L32

error[E0308]: try expression alternatives have incompatible types
  --> src\lib.rs:40:12
   |
40 |         Ok(global.dyn_into::<Window>()?)
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `web_sys::WorkerGlobalScope`, found struct `web_sys::Window`
   |
   = note: expected type `web_sys::WorkerGlobalScope`
              found type `web_sys::Window`

error: aborting due to previous error

so only boxed dynamic return type works in this case ... or an enum.

@wellcaffeinated
Copy link

Hi I'm attempting to load my code in a webworker and running into this. Is there a timeline for release?

@alexcrichton
Copy link
Contributor

I'm going to close this since we're unlikely to add layers of abstraction in web-sys or js-sys itself, and otherwise this has been quiet for quite some time. If there's follow-up items to take care of, please feel free to open a separate issue!

Globidev added a commit to Globidev/reqwest that referenced this issue Mar 17, 2020
This looks pretty hacky until something is done regarding rustwasm/wasm-bindgen#1046
Globidev added a commit to Globidev/reqwest that referenced this issue Mar 17, 2020
This looks pretty hacky until something is done regarding rustwasm/wasm-bindgen#1046
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 19, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global by binding to the global
`setInterval` and `clearInterval` functions directly.
This uses the same approach as used by gloo-timers implemented in
[rustwasm/gloo#185](rustwasm/gloo#185) and
[rustwasm/gloo#283](rustwasm/gloo#283) given
the `web-sys` lack of interes to support this (see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).

Co-authored-by: sisou <hello@soerenschwert.de>
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 19, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by binding to the
global `setInterval` and `clearInterval` functions directly.
This uses the same approach used by gloo-timers implemented in
[rustwasm/gloo#185](rustwasm/gloo#185) and
[rustwasm/gloo#283](rustwasm/gloo#283) given
the `web-sys` lack of interes to support this (see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).

Co-authored-by: sisou <hello@soerenschwert.de>
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 20, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interes to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 20, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interes to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 20, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interes to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 20, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interest to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 20, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interest to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
jsdanielh added a commit to jsdanielh/rust-libp2p that referenced this issue Nov 20, 2023
Add support different WASM environments (such as workers, NodeJS)
that don't have a `window` global. This is done by introducing a
`WebContext` `enum` that abstracts and detects the `Window` vs the
`WorkerGlobalScope` API.
This is done due to the `web-sys` lack of interest to support this
(see discussion in
[this issue](rustwasm/wasm-bindgen#1046)).
mergify bot pushed a commit to libp2p/rust-libp2p that referenced this issue Nov 21, 2023
We introduce a `WebContext` `enum` that abstracts and detects the `Window` vs the `WorkerGlobalScope` API.

Related: rustwasm/wasm-bindgen#1046.

Pull-Request: #4889.
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

No branches or pull requests

5 participants