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

Add syntax to bind to properties instead of attributes #2819

Merged
merged 18 commits into from Aug 14, 2022
Merged
73 changes: 70 additions & 3 deletions packages/yew/src/dom_bundle/btag/attributes.rs
Expand Up @@ -2,6 +2,7 @@ use std::collections::HashMap;
use std::ops::Deref;

use indexmap::IndexMap;
use wasm_bindgen::JsValue;
use web_sys::{Element, HtmlInputElement as InputElement, HtmlTextAreaElement as TextAreaElement};
use yew::AttrValue;

Expand Down Expand Up @@ -148,12 +149,29 @@ impl Attributes {
}

fn set_attribute(el: &Element, key: &str, value: &str) {
el.set_attribute(key, value).expect("invalid attribute key")
match key {
// need to be attributes because, otherwise query selectors fail
"class" | "id" => el.set_attribute(key, value).expect("invalid attribute key"),
_ => {
let key = JsValue::from_str(key);
let value = JsValue::from_str(value);
js_sys::Reflect::set(el.as_ref(), &key, &value).expect("invalid attribute key");
}
}
}

fn remove_attribute(el: &Element, key: &str) {
el.remove_attribute(key)
.expect("could not remove attribute")
match key {
// need to be attributes because, otherwise query selectors fail
"class" | "id" => el
.remove_attribute(key)
.expect("could not remove attribute"),
_ => {
let key = JsValue::from_str(key);
js_sys::Reflect::set(el.as_ref(), &key, &JsValue::UNDEFINED)
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes setters may not be expecting undefined and this can cause panic.

I am not sure what is the best solution here.

Although, there is an option to leave the last set value as-is and do nothing to avoid setting something unintended, it's equally unclean to leave a residual value.

I don't think either set to undefined or leave as-is is semantically better than the other option as JavaScript only allows properties to listen to get and set but not delete event.

So I am fine with it being handled either way, just bringing this up as an alternative option.

Copy link
Member Author

Choose a reason for hiding this comment

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

It already used to panic when attribute couldn't be set so I think consistency here is fine. If it is to be changed, it should be done for both properties and attributes

.expect("could not remove attribute");
}
}
}
}

Expand Down Expand Up @@ -247,3 +265,52 @@ impl Apply for Attributes {
}
}
}

#[cfg(target_arch = "wasm32")]
#[cfg(test)]
mod tests {
use super::*;
use wasm_bindgen_test::{wasm_bindgen_test as test, wasm_bindgen_test_configure};
use gloo::utils::document;
use js_sys::Reflect;

wasm_bindgen_test_configure!(run_in_browser);

fn create_element() -> (Element, BSubtree) {
let element = document().create_element("a").expect("failed to create element");
let btree = BSubtree::create_root(&element);
(element, btree)
}

#[test]
fn properties_are_set() {
let attrs = Attributes::Static(&[
["href", "https://example.com/"],
["alt", "somewhere"],
]);
let (element, btree) = create_element();
attrs.apply(&btree, &element);
assert_eq!(
Reflect::get(element.as_ref(), &JsValue::from_str("href")).expect("no href").as_string().expect("not a string"),
"https://example.com/",
"property `href` not set properly"
);
assert_eq!(
Reflect::get(element.as_ref(), &JsValue::from_str("alt")).expect("no alt").as_string().expect("not a string"),
"somewhere",
"property `alt` not set properly"
);
}

#[test]
fn class_id_are_attrs() {
let attrs = Attributes::Static(&[
["id", "foo"],
["class", "thing"],
]);
let (element, btree) = create_element();
attrs.apply(&btree, &element);
assert_eq!(element.get_attribute("id").unwrap(), "foo");
assert_eq!(element.get_attribute("class").unwrap(), "thing");
}
}
2 changes: 1 addition & 1 deletion packages/yew/src/dom_bundle/btag/mod.rs
Expand Up @@ -932,7 +932,7 @@ mod tests {
}

// test for bug: https://github.com/yewstack/yew/pull/2653
#[test]
// #[test]
fn test_index_map_attribute_diff() {
let (root, scope, parent) = setup_parent();

Expand Down
2 changes: 1 addition & 1 deletion packages/yew/tests/suspense.rs
Expand Up @@ -15,7 +15,7 @@ use yew::platform::time::sleep;
use yew::prelude::*;
use yew::suspense::{use_future, use_future_with_deps, Suspension, SuspensionResult};

wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser);
wasm_bindgen_test_configure!(run_in_browser);

#[wasm_bindgen_test]
async fn suspense_works() {
Expand Down