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

Binding value require type annotations in some cases. #1176

Closed
vldm opened this issue Jun 13, 2023 · 8 comments · Fixed by #1636
Closed

Binding value require type annotations in some cases. #1176

vldm opened this issue Jun 13, 2023 · 8 comments · Fixed by #1636
Labels
enhancement New feature or request

Comments

@vldm
Copy link
Contributor

vldm commented Jun 13, 2023

As was proposed in #1140 (comment) creating a new issue with
complete-but-not-compiling example.

In this example, i trying to create a component that request data from external storage, the query is bound to Type as in ORM.

Instead of using real db, i added mocked version, to simlify example.
The full source code can be found at https://github.com/vldm/leptos-non-working-orm/blob/master/src/lib.rs, but to make issue independent i also attach its compact version:

use leptos::*;
 pub trait SelectFromDb {
      fn mock_select_all() -> Vec<(u32, Self)>
      where Self: Sized;
}
#[component]
fn DisplayList<T, FV, V>(cx: Scope, children: FV) -> impl IntoView
where
    FV: Fn(Scope, &T) -> V + 'static,
    V: IntoView,
    T: SelectFromDb + 'static + Clone,
{
    let data: Vec<(u32, T)> = <T as SelectFromDb>::mock_select_all(/* some limits, and other params */);
    let (data, _) = create_signal(cx, data);

    view! {
        cx,
        <ul>
        <For each=data key=|i|i.0 view=move |cx, (_,v)| {
            view!{ cx, <li> {children(cx, &v)} </li> }
        } />
        </ul>
    }
}

#[derive(Clone)]
struct MyObject {
    name: String,
    age: u32,
}
impl SelectFromDb for MyObject {
    fn mock_select_all() -> Vec<(u32, Self)>
    where
        Self: Sized,
    {
        vec![
            (0,MyObject { name: "Ivan".to_string(), age: 32,},),
            /* .. */
        ]
    }
}

#[component]
pub fn App(cx: Scope) -> impl IntoView {
    view! { cx,
        "I have a table and want to render it:"
        <DisplayList
            bind:my_object
        >
            {&my_object.name}" => "{my_object.age}
        </DisplayList>
    }
}

Leptos Dependencies
Please copy and paste the Leptos dependencies and features from your Cargo.toml.

For example:

[dependencies]
leptos = {git = "https://github.com/leptos-rs/leptos" }
console_log = "1"
log = "0.4"
console_error_panic_hook = "0.1.7"
tuple="0.5.1"

To Reproduce
Steps to reproduce the behavior:

  1. clone repo https://github.com/vldm/leptos-non-working-orm
  2. checkout to master
  3. cargo check
  4. See error

Screenshots
some

Related issue

During writing this example, i also found that #[component] macro can't implement props builder for this component, because value T is only bound to Fn arguments, so compiller ask to use PhantomData for each unused bound.

Possible solutions

  1. creating a type alias for component, with that will declare
type MyDisplayList<U,V> = DisplayList<MyObject, U, V>;

But because component is fn not a type, i cant create a type alias.
2. Type in component probably can solve the issue too

view!{cx,
        <DisplayList<T, _,_>
             bind:my_object
         >
            {my_object.name} = {my_object.age}
         </DisplayList>
}

I personaly doesn't like <> inside tags, because they can confuse users, but yew use it, and i have seen an issue in syn-rsx with proposal to add this.
3. writing fn alias with type generics declaration

    fn MyDisplayList<FV, V>(cx:Scope, props: DisplayListProps<MyObject, FV, V>) -> impl IntoView    
    where
        FV: Fn(Scope, &MyObject) -> V + 'static,
        V: IntoView,
    {
        DisplayList(cx, props)
    }

this is the only one, that work for me in current leptos, can be found at https://github.com/vldm/leptos-non-working-orm/blob/working-fix
But for me it is the most ugly one.
4. New syntax, that i proposed in original dissucssion bind(my_object: &MyObject) instead of bind:my_object
The only problem that i found only one two places where this syntax is required:

  • bind to children attributes
  • declare your component props in declarative manner <script lang="rust" component:name="Foo" props(initial_value: i32, step: i32)> /* rust code */ <..

There is also other aplication for bind, but usually they are not require any syntax, or syntax that they require is not same as in ClosureArgs, some of examples:

@gbj gbj added the enhancement New feature or request label Jun 13, 2023
@gbj
Copy link
Collaborator

gbj commented Jun 13, 2023

Ah yeah I see, it's an issue of type inference here because the trait T::something is used inside the component, and no data of type T is provided from outside.

Agreed that this does not compile:

<DisplayList
  bind:my_object
>
  {&my_object.name}" => "{my_object.age}
</DisplayList>

However I'd note that this does compile, because it means that the compiler can correctly infer the type of my_object

<DisplayList
  bind:my_object
>
  {let _: &MyObject = my_object;}
  {&my_object.name}" => "{my_object.age}
</DisplayList>

Perhaps that's too clever.

@lpotthast
Copy link
Contributor

This problem will always occur when writing a component that is generic over some T while not receiving a T directly as a prop, as Rusts type inference won't resolve that.

So

  • fn MyComponent<T>(cx: Scope, value: T) works
  • fn MyComponent<T>(cx: Scope, value: Signal<T>) works
  • fn MyComponent<T>(cx: Scope, value: T::MyAssociatedType) does not work
  • fn MyComponent<T>(cx: Scope, value: Signal<T::MyAssociatedType>) does not work and
  • fn MyComponent<T>(cx: Scope) does not work

Another solution to the problem would be to go the PhantomData route, as

fn MyComponent<T>(cx: Scope, _phantom: PhantomData<T>) works, as it falls in first/second category in the above list.
You can instantiated such a component with

    <MyComponent  _phantom={ PhantomData::<T>::default() } />

I have used this pattern for a few library components.

The code-block inside the children is clever! Never thought about that.

It seems that there is the benefit of not requiring any children prop using the PhantomData prop.
Having the prop with its generated documentation should make it a bit less inconvenient for users of such a component, should that component be public.

@gbj
Copy link
Collaborator

gbj commented Jun 23, 2023

@vldm Given these two options, do you think there's a need for additional support in the view macro itself? To me, providing the type in a block or via PhantomData upholds the nice separation between the view syntax proper (mostly XML-like) and blocks (ordinary Rust), and this allows us to solve the problem without the burden of additional syntax support in the macro. I'm open other points of view, though.

@vldm
Copy link
Contributor Author

vldm commented Jun 23, 2023

@gbj I fully support the idea of separating xml-like part from rust blocks. But I'm not drastically tuned, because I think some simplifications are appropriate (for example, expressions without curly braces in attribute values)

Both provided solutions look like hacks to me. Yes, they solve the issue, but they could be not obviuos for users.
Also both of them forcing users to write some rust code that doesn't implement any logic - just to solve the issue with the declaration.

I think, implementing generics in component is probably a better way to make original code compilable, especially when you consider that in other frameworks this has already been done. (But as @lpotthast mention, it doesn't solve the case where some associated type of generic is used)

As for the "binding syntax", i think that the argument for changing it will only be the need to use bind: outside of children fn (for example in node_ref binding, or in binding props).
I still like to see new syntax in <For > component usecase, because it allows "destructing" of item, but i will not insist.

From rstml perspective, i have plan to implement both: generics and "fn binding syntax", but currently a bit busy with my job, so have no time to implement generics. And if you need any of these features in a leptos view/template - I'd be happy to add supportthese.

@gbj
Copy link
Collaborator

gbj commented Jun 23, 2023

Cool, didn't know Yew did this (allow generics in component names). I'm very comfortable with that solution. Not urgent of course.

view! { cx,
    <MyGenericComponent<i32> data=123/>
};

@gbj
Copy link
Collaborator

gbj commented Sep 4, 2023

The solution mentioned above (allow generics on component names) has been implemented in rstml and now supported in Leptos in #1636.

view! {
    <MyGenericComponent<i32> data=123/>
};

@benwis
Copy link
Contributor

benwis commented Sep 21, 2023

The solution mentioned above (allow generics on component names) has been implemented in rstml and now supported in Leptos in #1636.

view! {
    <MyGenericComponent<i32> data=123/>
};

Does this extrapolate out to multiple generics?

view! {
    <MyGenericComponent<i32, u64, String> data=123, data2=456, data3="Hi".to_string()/>
};

@vldm
Copy link
Contributor Author

vldm commented Sep 22, 2023

@benwis I didn't test it in leptos.

But in rstml it can parse array of generics (and even lifetime generics https://docs.rs/syn/2.0.28/syn/struct.Generics.html)

In my opinion if it can't in leptos or parser works in controversial way with regular rust generics syntax - it can be assumed as a bug.

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

Successfully merging a pull request may close this issue.

4 participants