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

feat: variable bindings on components and slots #1140

Merged
merged 1 commit into from
Jun 3, 2023

Conversation

ilmmatias
Copy link
Contributor

This should close #943.

@gbj
Copy link
Collaborator

gbj commented Jun 3, 2023

Yes, this looks great.

@gbj
Copy link
Collaborator

gbj commented Jun 5, 2023

In case it's not obvious to anyone how this works, here's an example adapting the <Await/> component (I'll probably actually do this)

Basically rather than children being constrained to FnOnce(Scope) -> impl IntoView the children can take additional arguments as defined, and you add bind:{variable name} in the view, in order, to be able to use those variables that are passed in.

pub fn Await<T, Fut, FF, VF, V>(
    cx: Scope,
    /// A function that takes a [`Scope`] and returns the [`Future`](std::future::Future) that
    /// will the component will `.await` before rendering.
    future: FF,
    /// If `true`, the component will use [`create_blocking_resource`], preventing
    /// the HTML stream from returning anything before `future` has resolved.
    #[prop(optional)]
    blocking: bool,
    /// A function that takes a [`Scope`] and a reference to the resolved data from the `future`
    /// renders a view.
    children: VF,
) -> impl IntoView
where
    Fut: std::future::Future<Output = T> + 'static,
    FF: Fn(Scope) -> Fut + 'static,
    V: IntoView,
    VF: Fn(Scope, &T) -> V + 'static,
    T: Serializable + 'static,
{
    let res = if blocking {
        create_blocking_resource(cx, || (), move |_| future(cx))
    } else {
        create_resource(cx, || (), move |_| future(cx))
    };
    let view = store_value(cx, children);
    view! { cx,
        <Suspense fallback=|| ()>
            {move || res.with(cx, |data| view.with_value(|view| view(cx, data)))}
        </Suspense>
    }
}

Now it can be used like this

view! { cx,
    <Await
        future=|_| fetch_monkeys(3)
        bind:data
    >
        <p>{*data} " little monkeys, jumping on the bed."</p>
    </Await>
}

and I can provide whatever name to data that I want

view! { cx,
    <Await
        future=|_| fetch_monkeys(3)
        bind:stuff
    >
        <p>{*stuff} " little monkeys, jumping on the bed."</p>
    </Await>
}

And I even get nice rust-analyzer support
Screenshot 2023-06-05 at 9 14 49 AM

@vldm
Copy link
Contributor

vldm commented Jun 5, 2023

Hi guys, awesome work. I am very interested in all "declarative components" innovations.

But this PR was merged rapidly and left one discussion about application of new binding syntax unfinished.

I originally left example with it in #943 (comment)

  <For
   each=iter
   key=|v| v
   bind(item: u32) // <== new syntax
   >
    <p> {item} </p>
  </For>

I already made PR to rstml with implementation rs-tml/rstml#17
But still unsure is it worth to change attribute parsing to support it (because it's looks uncommon for regular HTML).
And need your opinion about it.

Why we need new syntax, and what does it solve?

  1. Generic code

@gbj In your example

view! { cx,
    <Await
        future=|_| fetch_monkeys(3)
        bind:stuff
    >
        <p>{*stuff} " little monkeys, jumping on the bed."</p>
    </Await>
}

fetch_monkeys() is known to return i32, but if you are using generic code, rust not always can infer type, and you need to write it.

So supporting special bind syntax, can enhance usability
imagine fn get_typed_resource<T>() -> Future<Item = T>, to

view! { cx,
    <Await
        future=|_| get_typed_resource::<u32>() // this turbofish can be removed
        bind:stuff // bind(stuff: &u32)  can be used instead
    >
        <p>Received: {*stuff}</p>
    </Await>
}
  1. Destructing of type

For iterating, over big struct, or when part of tuple is not so important, destructing can make code cleaner.

struct Person {
    id: UID
    name: String,
    age: u8,
}

let people_iter = /* ... */
/* ... */
view!{
  <For 
   each=people_iter
   key=|id| id
   bind(Person { name, age, .. }: Person)
  >
    <p> {name} - {age} </p>
  </For>
}
  1. Binding variable by tag attribute possition can be unexpected.

Without types, users can mess with binding order especially especially if they know, that in html attribute order has no meaning.

<Component bind:x bind:y >
{x} {y}
</Component>

@gbj @yuuma03 On your opinions, should we add new syntax, or current bind:var, is look good enough?
And second question should i move this comment to separate issue?

@gbj
Copy link
Collaborator

gbj commented Jun 5, 2023

@vldm In your example, personally I think disambiguating with the turbofish is actually preferable, as it puts the specification of the type firmly in the realm of normal Rust code, rather than in the view DSL.

I could get on board with the (ident: type) syntax as an optional way to specify the type. I'd be curious to see an example in which it's necessary, strictly speaking.

@vldm
Copy link
Contributor

vldm commented Jun 5, 2023

@gbj

In your example, personally I think disambiguating with the turbofish is actually preferable, as it puts the specification of the type firmly in the realm of normal Rust code, rather than in the view DSL.

I see, yes currently attribute is a mix of html attribute with value expected to be valid rust expression.
And in bind syntax, there is no expression - only "pattern", which is not common used outside of fn calls/closures, etc.

I'd be curious to see an example in which it's necessary, strictly speaking.

It's neccessary in the same places where it necessary for closure.

Modification of 1.
Where factory is created inside component, and no chances to use turbofish.

For example, imagine generic component that draw sql table(or any row from it) which doesn't know it's output type (common practice for ORM). The component is only know how to draw row or table, and use children_fmt to draw

pub fn DrawTable<T, VF, V>(
    cx: Scope,
    children_fmt: VF,
) -> impl IntoView
where
    V: IntoView,
    VF: Fn(Scope, &T) -> V + 'static,
    T: Serializable + 'static,
{ 
    let res = create_resource(cx, || (), move |_| /* Some call to db */);
    // draw each row using children_fmt()
}

It can be reused to draw any data from db, but i am not sure how in current design on caller site, i can set T to be specific type.
If i could use some kind of type DrawMyTable<VF, V> = DrawTable<MyTable, VF, V> it probably solve the issue.

I could get on board with the (ident: type) syntax as an optional way to specify the type

I am not sure that supporting two style is realy a good idea, because it can confuse new users.

What i'd prefer is to keep bind namespace for future sugar.
For example, bind:children/bind::output can be used to pass data to children, and some other values can be used as in svelte (bind:value, bind:group) to bind inner data to some variable/signal.

But anyway, last word is on you. If you think that it will not confuse users, i can create a PR with it, after carefully testing bind syntax in rstml.

@gbj gbj mentioned this pull request Jun 5, 2023
14 tasks
@gbj
Copy link
Collaborator

gbj commented Jun 10, 2023

@vldm Since this syntax hasn't been released yet, I'm comfortable with changes since they won't be semver breaking at all. However, I'd suggest we take the conversation to a new issue. I understand the ORM example you're raising in theory, but am curious about how it would actually work (or not work) in practice. If you open an issue with a complete-but-not-compiling example I'd be happy to talk through the options.

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.

Passing props to children in slots
3 participants