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

inconsistent directive behaviour #2248

Closed
blorbb opened this issue Jan 31, 2024 · 1 comment · Fixed by #2249
Closed

inconsistent directive behaviour #2248

blorbb opened this issue Jan 31, 2024 · 1 comment · Fixed by #2249

Comments

@blorbb
Copy link
Contributor

blorbb commented Jan 31, 2024

Describe the bug
The implementation of directives on the macro seems to have somewhat inconsistent behaviour.

The current implementation is this:

pub(crate) fn directive_call_from_attribute_node(
    attr: &KeyedAttribute,
    directive_name: &str,
) -> TokenStream {
    let handler = format_ident!("{directive_name}", span = attr.key.span());

    let param = if let Some(value) = attr.value() {
        quote! { #value.into() }
    } else {
        quote! { () }
    };

    quote! { .directive(#handler, #param) }
}

I'm not sure why .into() is called on #value - was this a deliberate decision to automatically call .into()? This might be a good API choice, so that many different inputs can be used, without the directive author needing to make the parameter impl Into<MyParameter>. This should also be documented, something I can add to the book once this is clarified.

However, this is inconsistent as into is not called on the no-value alternative, (). There may be cases where there is a sensible default if the user doesn't provide any parameter, but right now the directive author can't add a impl From<()> for MyParameter to accept it.

❗ I'm not sure if this is a breaking change to add .into() though.

Another bit: the first branch should be (#value).into() so that into is called on the whole value passed in, not just added to the end. For example, this wouldn't work:

struct Param;
impl<T: Fn() -> i32> From<T> for Param {
    // ...
}

fn directive(el: HtmlElement<AnyElement>, p: Param) { /* ... */ }

// this doesn't work: expands to `|| 10.into()`
view! {
    <div use:directive=|| 10 />
}
// currently have to do this to get `{|| 10}.into()`
view! {
    <div use:directive={|| 10} />
}

I think fixing this won't be a breaking change.

I can make a PR to fix these issues, just wanted to check what the intended behaviour was and if this might be a breaking change.

Leptos Dependencies

leptos = { version = "0.6", features = ["csr"] }

To Reproduce
described above.

Expected behavior
also described above.

Screenshots
N/A

Additional context
N/A

@gbj
Copy link
Collaborator

gbj commented Jan 31, 2024

impl From<()> for MyParameter

This is clever and a great point.

the first branch should be (#value).into() so that into is called on the whole value passed in

You are right, and this is a good catch. It might require some additional #[allow(unused_parens)] somewhere to suppress warnings in the cases where they aren't needed; not sure, worth checking if you're working on it.

I can make a PR to fix these issues, just wanted to check what the intended behaviour was and if this might be a breaking change.

That would be great!

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 a pull request may close this issue.

2 participants