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

Notifications API library #52

Open
Aloso opened this issue Mar 27, 2019 · 7 comments
Open

Notifications API library #52

Aloso opened this issue Mar 27, 2019 · 7 comments

Comments

@Aloso
Copy link

Aloso commented Mar 27, 2019

Summary

Make an API for displaying notifications (see MDN documentation).

Motivation

The event handlers in web-sys accept a Option<&Function>, which is ugly to work with. Also, it is missing some options.

Detailed Explanation

I propose to use Rust closures instead of Function. Also I'd like a function requesting permission to display notifications, that returns a Future.

Notification::builder() returns a NotificationBuilder, whose build function is called show():

Notification::request_permission()
    .map(|_| {
        let notification = Notification::builder()
            .title("Foo")
            .body("Bar")
            .icon("notification.png")
            .badge("notification-badge.png")
            .image("img.png")
            .silent(true)
            .show();

        on(&notification, move |e: ClickEvent| unimplemented!())
    })

Drawbacks, Rationale, and Alternatives

I don't know of any drawbacks. However, I'm not familiar with other frameworks, so please tell me if there are better alternatives.

Unresolved Questions

The syntax of the event listeners, which is discussed in #43

@fitzgen
Copy link
Member

fitzgen commented Mar 27, 2019

Notification::builder() returns a NotificationBuilder, whose build function is called show():

The existing web_sys::NotificationOptions is almost exactly this, except missing the show finishing method that constructs the notification with the configured options. I could imagine either a wrapper that derefs to web_sys::NotificationOptions or a new builder that duplicates all the configuration methods. I guess the latter is probably easier for users.

Also I'd like a function requesting permission to display notifications, that returns a Future.

I see two basic designs for this future:

  1. Future<Item = bool, Error = ()> -- the bool item is true if permission is granted, and false if it is denied. Error is ignored. This provides a nice upgrade path for the new std::futures::Futures, where the Error associated item no longer exists.

  2. Future<Item = (), Error = ()> -- if the future polls to Ok(Async::Ready(())) then permission is granted. If it polls to Err(()) then permission is denied.

I don't have strong opinions between the two.

Another question is whether we want to support the (deprecated) callback API for requesting permissions or not. If we did, then Rust users who aren't using futures would have a way to use the API.

Yet another, higher-level, alternative is to make creating notifications require a capability object that can only get obtained via requesting permissions and having it be granted. Something like this sketch:

pub fn request_permission() -> impl Future<Item = NotificationCapability, Error = PermissionDenid> { ... }

pub struct NotificationCapability { ... }
impl NotificationCapability {
    pub fn new_notification(&self) -> NotificationBuilder { ... }
}

It is unclear to me whether this capabilities-based approach would benefit from being a layer on top of the others or not. It seems like it would be a very thin layer, and the whole point of the capabilities approach is to remove incorrect usage at compile time, so being able to get an almost-identical API that allows that incorrect usage seems strange.


I guess that I've sort of convinced myself by writing all this out that:

  • we should pursue the capabilities-based API design (that is not layered on another mid-level layer, but is just right on top of the web-sys bindings)
  • we should write our own builder that wraps web_sys::NotificationOptions instead of deref'ing to it

@Aloso
Copy link
Author

Aloso commented Mar 27, 2019

Thanks for the feedback.

I see two basic designs for this future:

I'm in favor of option 2.

we should pursue the capabilities-based API design

I think it would be easier if we use a Future<Item=NotificationBuilder, Error=()> and make it impossible to create a NotificationBuilder directly. This means that the user has to call request_permission() every time he needs a NotificationBuilder:

Notification::request_permission()
    .map(|builder| {
        builder
            .title("Foo")
            .body("Bar")
            .show();
    })

This also works if permissions are revoked in the browser after they were granted.

Another question is whether we want to support the (deprecated) callback API for requesting permissions or not. If we did, then Rust users who aren't using futures would have a way to use the API.

I haven't thought about that. Since it's deprecated, maybe we could use a callback in Rust, and a javascript Promise behind the scenes:

Notification::request_permission_map(|builder| {
    builder
        .title("Foo")
        .body("Bar")
        .show();
});

we should write our own builder that wraps web_sys::NotificationOptions instead of deref'ing to it

About that: web_sys::NotificationOptions doesn't support all options in the specification, e. g. silent. If we want to support all options, how should be proceed?

@fitzgen
Copy link
Member

fitzgen commented Mar 28, 2019

This also works if permissions are revoked in the browser after they were granted.

Interesting point!

If we wanted to guarantee that the builder is used on the same tick that the permissions request was granted, we could add a phantom 'a to the builder and require the user provides a F: for<'a> FnOnce(NotificationBuilder<'a>) and then they can't stash it on the side anywhere. This does mean that we have to use callbacks in the API though, since I don't think you can do this trick without them.

About that: web_sys::NotificationOptions doesn't support all options in the specification, e. g. silent. If we want to support all options, how should be proceed?

It means that the Web IDL interfaces that web-sys is using doesn't have that yet. Maybe it is relatively new, or we might have accidentally removed it when removing Firefox-specific things (we took the interfaces from mozilla-central)

https://github.com/rustwasm/wasm-bindgen/blob/c517b0082505aab313f844c3268acf2aa6944d41/crates/web-sys/webidls/enabled/Notification.webidl#L17

Assuming it is a standard option, then we would send a PR upstream to add it to that interface, and wait for a new version of web-sys to be published before we started using it here.

@Aloso
Copy link
Author

Aloso commented Mar 28, 2019

@fitzgen

we took the interfaces from mozilla-central

Yeah, Firefox doesn't support all the options (but Chrome does). They aren't that new: For example, silent is supported since Chrome 43 (released 2015). vibrate is supported since Chrome 53.

@Aloso
Copy link
Author

Aloso commented Mar 28, 2019

If we wanted to guarantee that the builder is used on the same tick that the permissions request was granted

I wouldn't call that a priority. It doesn't happen too often that permissions are revoked, and even if a Notification is created without permission (provided that the programmer messed up), usually nothing bad happens.

@Aloso
Copy link
Author

Aloso commented Mar 30, 2019

@fitzgen do you know what's the best way to test this? I can't grant permissions in wasm-bindgen-test.

@fitzgen
Copy link
Member

fitzgen commented Apr 1, 2019

Probably to include some JS that monkey patches / overrides the Notification API and exposes a switch we can flick to make requests always granted and always forbidden.

Here is an (untested) sketch:

// notification-monkey-patch.js

let shouldGrantPermission = false;

window.Notification = class {
    static requestPermission() {
        return shouldGrantPermission ? Promise.resolve() : Promise.reject();
    }

    // ...
};

window.setShouldGrantPermission = should => shouldGrantPermission = should;
// crates/notification/tests/web.rs

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
extern "C" {
    #[wasm_bindgen(js_name = setShouldGrantPermission)]
    fn set_should_grant_permission(should: bool);
}

@MTRNord MTRNord mentioned this issue May 27, 2020
9 tasks
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

2 participants