Skip to content

Conversation

@kirillsemyonkin
Copy link
Contributor

@kirillsemyonkin kirillsemyonkin commented Nov 1, 2023

Description

Partially implements features from discussion #3477. This PR adds Dynamic Prop Labels, so that anyone can write their favorite attributes like HTMX's hx-on:click:

html! {
    <div "hx-on:click"="alert('Clicked!')">{ "Click" }</div>
    <div { "hx-on:click" }={ "alert('Clicked!')" }>{ "Click" }</div>
}

It works by using new PropLabel enum everywhere, which can be either a Static HtmlDashedName like before, or Dynamic Expr. When parsing, if it meets = token after the expression group, it will read a value following it instead of assuming that it is shorthand syntax.

Checklist

  • I have reviewed my own code - what does that mean? Is this always checked for everyone?
  • I have added tests

@github-actions
Copy link

github-actions bot commented Nov 1, 2023

Visit the preview URL for this PR (updated for commit 5fc9058):

https://yew-rs-api--pr3509-dyn-prop-0vcn3vrd.web.app

(expires Mon, 04 Dec 2023 08:06:38 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@kirillsemyonkin
Copy link
Contributor Author

kirillsemyonkin commented Nov 1, 2023

My local setup told me nothing about which Rust version is used, wow...

Fixing 1.64.0 compiler support shortly I guess, there is nothing special, just Result::is_ok_and.

@github-actions
Copy link

github-actions bot commented Nov 1, 2023

Size Comparison

Details
examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 100.292 100.292 0 0.000%
boids 173.724 173.724 0 0.000%
communication_child_to_parent 92.738 92.738 0 0.000%
communication_grandchild_with_grandparent 105.761 105.761 0 0.000%
communication_grandparent_to_grandchild 101.012 101.012 0 0.000%
communication_parent_to_child 89.088 89.088 0 0.000%
contexts 106.004 106.004 0 0.000%
counter 86.120 86.120 0 0.000%
counter_functional 86.515 86.515 0 0.000%
dyn_create_destroy_apps 88.930 88.930 0 0.000%
file_upload 99.988 99.988 0 0.000%
function_memory_game 172.357 172.357 0 0.000%
function_router 348.660 348.660 0 0.000%
function_todomvc 161.180 161.180 0 0.000%
futures 229.034 229.034 0 0.000%
game_of_life 110.118 110.118 0 0.000%
immutable 185.424 185.424 0 0.000%
inner_html 79.896 79.896 0 0.000%
js_callback 109.511 109.511 0 0.000%
keyed_list 199.562 199.562 0 0.000%
mount_point 82.784 82.784 0 0.000%
nested_list 113.870 113.870 0 0.000%
node_refs 90.325 90.325 0 0.000%
password_strength 1750.046 1750.046 0 0.000%
portals 93.458 93.458 0 0.000%
router 317.565 317.565 0 0.000%
simple_ssr 140.313 140.313 0 0.000%
ssr_router 385.893 385.893 0 0.000%
suspense 115.705 115.705 0 0.000%
timer 88.555 88.555 0 0.000%
timer_functional 97.918 97.918 0 0.000%
todomvc 141.333 141.333 0 0.000%
two_apps 85.821 85.821 0 0.000%
web_worker_fib 134.756 134.756 0 0.000%
web_worker_prime 184.976 184.976 0 0.000%
webgl 82.500 82.500 0 0.000%

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

github-actions bot commented Nov 1, 2023

Benchmark - SSR

Yew Master

Details
Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.358 295.919 290.165 2.026
Hello World 10 504.055 564.258 515.305 18.384
Function Router 10 1605.011 1623.532 1613.642 5.706
Concurrent Task 10 1005.584 1006.625 1006.134 0.373
Many Providers 10 1113.755 1164.546 1137.840 17.422

Pull Request

Details
Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.261 289.772 289.505 0.156
Hello World 10 476.348 495.162 479.349 5.678
Function Router 10 1626.899 1648.855 1634.601 8.137
Concurrent Task 10 1004.922 1006.348 1005.895 0.485
Many Providers 10 1124.838 1165.348 1143.790 12.075

@kirillsemyonkin
Copy link
Contributor Author

Uh sorry, will check what it fails on tomorrow

@kirillsemyonkin
Copy link
Contributor Author

Wow, I passed even the benchmark lol
I still need help with writing tests for this.

Allows not only literals, e.g. "hx-on:click", but all expressions
@ranile
Copy link
Member

ranile commented Nov 2, 2023

You need both passing and failing tests for it. You can copy a -fail.rs and -pass.rs from yew-macro/tests/html_macro and modify it as needed. To regenerate stderr files, you can run tests with TRYBUILD=overwrite environment variable

@kirillsemyonkin
Copy link
Contributor Author

If there are any // FIXMEs in my PR, I also need them reviewed

cecton
cecton previously approved these changes Nov 6, 2023
Copy link
Contributor

@cecton cecton left a comment

Choose a reason for hiding this comment

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

Awesome! I need this. At the moment I'm building the component linearGradient manually and it's such a pain

#[function_component(LinearGradient)]
pub fn linear_gradient(
    Props {
        id,
        gradient_transform,
        children,
    }: &Props,
) -> Html {
    let mut vtag = yew::virtual_dom::VTag::new("linearGradient");
    vtag.add_children(children.clone());
    vtag.add_attribute("id", id.clone());
    if let Some(x) = gradient_transform.clone() {
        vtag.add_attribute("gradientTransform", x);
    }
    vtag.into()
}

Now with autoprops + dyn prop labels, I can simplify this code a lot

@kirillsemyonkin
Copy link
Contributor Author

kirillsemyonkin commented Nov 6, 2023

I tried to add following to the dyn-prop-pass.rs test:

    // property literal
    _ = ::yew::html! { <span ~"hx-on:click"="alert('Clicked!')" /> };
    _ = ::yew::html! { <span ~"hx-on:click"="alert('Clicked!')" ~"hx-on:click"="alert('Clicked!')" /> };
    _ = ::yew::html! { <span ~{ "hx-on:click" }={ "alert('Clicked!')" } /> };
    _ = ::yew::html! { <span ~{ "hx-on:click" }={ "alert('Clicked!')" } ~{ "hx-on:click" }={ "alert('Clicked!')" } /> };

    // property expr
    _ = ::yew::html! { <span ~{ dyn_prop() }={ "alert('Clicked!')" } /> };
    _ = ::yew::html! { <span ~{ dyn_prop() }={ "alert('Clicked!')" } ~{ dyn_prop() }={ "alert('Clicked!')" } /> };

Tests (RUST_BACKTRACE=1 cargo +1.64.0 test -p yew-macro) greeted me with the following:

test tests\html_macro\dyn-prop-pass.rs [should pass] ... error
Test case failed at runtime.

STDERR:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
thread 'main' panicked at 'function not implemented on non-wasm32 targets', D:\RustDownloads\cargo\registry\src\github.com-1ecc6299db9ec823\wasm-bindgen-0.2.87\src\lib.rs:996:1       
stack backtrace:
   0: rust_begin_unwind
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library\std\src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library\core\src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52/library\core\src/panicking.rs:48:5
   3: wasm_bindgen::__wbindgen_string_new
             at D:\RustDownloads\cargo\registry\src\github.com-1ecc6299db9ec823\wasm-bindgen-0.2.87\src\lib.rs:41:17
   4: wasm_bindgen::JsValue::from_str
             at D:\RustDownloads\cargo\registry\src\github.com-1ecc6299db9ec823\wasm-bindgen-0.2.87\src\lib.rs:132:32
   5: <wasm_bindgen::JsValue as core::convert::From<&str>>::from
             at D:\RustDownloads\cargo\registry\src\github.com-1ecc6299db9ec823\wasm-bindgen-0.2.87\src\lib.rs:776:9
   6: <T as core::convert::Into<U>>::into
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\core\src\convert/mod.rs:550:9
   7: trybuild005::main
             at D:\RustProjects\yew\packages\yew-macro\tests\html_macro\dyn-prop-pass.rs:58:30
   8: core::ops::function::FnOnce::call_once
             at /rustc/a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52\library\core\src\ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

As I was trying to find out whether any tests would help me figure out how to test the feature, I performed a quick search on all tests, only to find out that nobody tested ~ properties. I will await on someone testing and fixing the feature before adding tests like this. (Edit: This is not blocking)

@its-the-shrimp
Copy link
Contributor

I performed a quick search on all tests, only to find out that nobody tested ~ properties

Either this is an outdated comment or you missed something, because there are Wasm tests that use ~, if you want to test how ~ works with dynamic prop labels, you can check out one of those Wasm tests and replace ~prop with ~"prop" or ~{ "prop" }. You can find all places where ~ is used by grepping ~[a-zA-Z]

fn to_tokens(&self, tokens: &mut TokenStream) {
tokens.extend(match self {
PropLabel::Static(dashed_name) => quote! {#dashed_name},
PropLabel::Dynamic(expr) => quote! {#expr}, // FIXME this probably wanted its group back
Copy link
Contributor

Choose a reason for hiding this comment

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

What did you mean by this fixme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no idea. probably that dynamic label { ... } should be emitted as { ... }, not as ..., and so that spans are preserved that group should be saved somehow or something

Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but it'd be better to write that to_tokens impl as

match self {
    Self::Static(name) => name.to_tokens(tokens),
    Self::Dynamic(expr) => expr.to_tokens(tokens),
}

to avoid allocating a temporary TokenStream just to feed it into another TokenStream

Comment on lines +158 to +161
tokens.extend(match self {
Key::Static(dashed_name) => quote! { #dashed_name },
Key::Dynamic(expr) => quote! { #expr },
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Suggested change
tokens.extend(match self {
Key::Static(dashed_name) => quote! { #dashed_name },
Key::Dynamic(expr) => quote! { #expr },
});
match self {
Key::Static(dashed_name) => dashed_name.to_tokens(tokens),
Key::Dynamic(expr) => expr.to_tokens(tokens),
}

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 this pull request may close these issues.

4 participants