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

Utility crate for common web_sys features #72

Closed
David-OConnor opened this issue Apr 7, 2019 · 3 comments · Fixed by #155
Closed

Utility crate for common web_sys features #72

David-OConnor opened this issue Apr 7, 2019 · 3 comments · Fixed by #155
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@David-OConnor
Copy link
Member

David-OConnor commented Apr 7, 2019

Summary

I propose adding a crate that wraps common web_sys features with a cleaner API.

Examples:

  • Wrap things like Window, Document and History in a way that handles failures automatically with expect_throw
  • Provide shims to get or set the value of input etc elements
  • Wrap console::log and console::error with a more flexible API

Motivation

The web_sys api can be verbose; high-level crates needn't each provide their own wrappers.

Detailed Explanation

Some common features of web_sys are verbose when used directly. It's likely that many Gloo crates will make use of them - let's settle on a general wrapping API higher-level crates can use. Example

Examples:

/// Convenience function to avoid repeating expect logic.
pub fn window() -> web_sys::Window {
    web_sys::window().expect_throw("Can't find the global Window")
}

/// Convenience function to access the web_sys DOM document.
pub fn document() -> web_sys::Document {
    window().document().expect_throw("Can't find document")
}

/// Convenience function to access the web_sys history.
pub fn history() -> web_sys::History {
    window().history().expect_throw("Can't find history")
}

/// Simplify getting the value of input elements; required due to the need to cast
/// from general nodes/elements to HTML__Elements.
pub fn get_value(target: &web_sys::EventTarget) -> String {
    if let Some(input) = target.dyn_ref::<web_sys::HtmlInputElement>() {
        return input.value();
    }
    if let Some(input) = target.dyn_ref::<web_sys::HtmlTextAreaElement>() {
        return input.value();
    }
    if let Some(input) = target.dyn_ref::<web_sys::HtmlSelectElement>() {
        return input.value();
    }
    "".into()
}

/// Similar to get_value
pub fn set_value(target: &web_sys::EventTarget, value: &str) {
    if let Some(input) = target.dyn_ref::<web_sys::HtmlInputElement>() {
        input.set_value(value);
    }
    if let Some(input) = target.dyn_ref::<web_sys::HtmlTextAreaElement>() {
        input.set_value(value);
    }
    if let Some(input) = target.dyn_ref::<web_sys::HtmlSelectElement>() {
        input.set_value(value);
    }
}

Drawbacks, Rationale, and Alternatives

The original web_sys APIs aren't that clumsy - this might clutter up Gloo without providing much benefit. Perhaps we want to use pattern matching to handle failures of web_sys::Window etc instead of using expect_throw.

It's easy to create a collection of helper functions that are intended to be general, but spread over a wide range of uses, some of which may not be common. It's possible that they may cover a smaller set of use-cases than intended. Thin wrappers in general can be seen as obfuscating the original, well-known API. Why build a new API that causes confusion with the one it wraps, when it's not much better?

Unresolved Questions

It's unclear what the scope of this should be. There are surely grey areas about what's appropriate here, and how cohesive/comprehensive it should be. What causes web_sys::Window etc to fail?

I'm certainly biased by my own use-cases. Do y'all think these are, in-fact useful, general wrappers that would see use in many/most Gloo crates?

I've only scratched the surface. What else should be added?

@Pauan
Copy link
Contributor

Pauan commented Apr 8, 2019

Provide shims to get or set the value of input etc elements

I'm not sure about this one. The casting is ugly, yes, but that's handled by other things like #43

I think we should hold off on this until there's a clear use case which isn't handled by another higher-level crate.

Wrap console::log and console::error with a more flexible API

I believe this is already handled by wasm-bindgen-console-logger and console_log. Can we make those crates more visible somehow?

Wrap things like Window, Document and History in a way that handles failures automatically with expect_throw

They should probably use unwrap_throw instead, since that results in lower file sizes (and in practice they shouldn't be throwing).

What causes web_sys::Window etc to fail?

I think the only case where it would fail is in WebWorkers (which run in a separate realm). It would also obviously fail in Node, but I think that's to be expected.

Personally, I don't see much use for a utility crate if all it's doing is providing thin wrappers for unwrapping window() and document().

I think there would need to be stronger motivation than that (e.g. a dozen small APIs which don't really fit into any other crate).

@Pauan
Copy link
Contributor

Pauan commented Apr 8, 2019

(To be clear, I'm not against the idea of a utility crate, and I agree that some things in web-sys could use some nicer wrappers, I just think this should incubate a bit first, it feels a bit too early to me)

@fitzgen
Copy link
Member

fitzgen commented Apr 8, 2019

@David-OConnor

I propose adding a crate that wraps common web_sys features with a cleaner API.

I feel like this is what all of Gloo is doing :-p

My gut reaction is that we should try to identify specific cases and create utility crates for those cases, rather than making a catch-all miscellaneous crate. For example with get_value and set_value, I think it may make sense to either create a general gloo_dom_tree crate for interacting with nodes and elements or perhaps a gloo_forms crate for specifically interacting with inputs, buttons, widgets, etc


@Pauan

I believe this is already handled by wasm-bindgen-console-logger and console_log. Can we make those crates more visible somehow?

FWIW, we have the console_log crate listed in the Rust and Wasm book's Crates You Should Know section.

We could also re-export it in gloo potentially.

A thing that I find myself doing for wasm projects is not relying on LTO to recognize that because logging is not enabled, it can remove the logging calls and their functions, and instead swapping out log! et al macros with dummys via a cargo feature:

Maybe it makes sense to centralize that dance in Gloo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants