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

Experimental component API #599

Closed

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented May 1, 2021

This is a proposal for a component API that offers:

  • Labeled required properties, ensured to be present at compile-time
  • Labeled optional properties
  • Polymorphic for all properties
  • A convenient consumer interface, through a stupidly simple macro that is very easy to understand

Perhaps more importantly though, the comp! macro provides the indirection necessary to intercept and create boundaries in the vdom tree, which enables state to be attached to nodes, local updates to be triggered, and paves the path towards implementing proper hooks.

I expect there to be some discussion around this, of course, but once we're reasonably confident in the design we could bring this in behind an experimental feature flag, and while it matures start looking into all the possible features it enables.

@glennsl glennsl force-pushed the feat/experimental-component-api branch 2 times, most recently from 262527b to e9b6da1 Compare May 1, 2021 19:31
- Labeled required properties, ensured to be present at compile-time
- Labeled optional properties
- Polymorphic for all properties
- A convenient consumer interface, through a stupidly simple macro that is very easy to understand
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't assume all people understand the same things the same way and that they are all experienced enough to understand it on the first glance. It's not recommended to say that something is "very simple" or similar stuff in documentation / tutorials in general. (I'm sure I've written something similar too in my examples/ tutorials / docs but it shouldn't be there).
So please change it to something like:

Suggested change
- A convenient consumer interface, through a stupidly simple macro that is very easy to understand
- A convenient consumer interface, through a simpler macro

OT: Please update also CHANGELOG and I think I can merge it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry, I didn't mean for this to be merged as-is, or for this text to be consumed by end-users just yet, but instead to facilitate discussion among contributors.

Before merging I'd like to move the macro into seed proper, behind an experimental feature flag, and perhaps add a Component trait so that it can't be used directly without setting up the (eventual) boundary, But as that would obscure the API a bit I wanted to do a round with just this first, to focus on the API only.

Since there doesn't seem to be any immediate concerns, however, I can do that later today and also try to fill in a bit more of the bigger picture.

@MartinKavik MartinKavik marked this pull request as draft May 2, 2021 11:15
@glennsl glennsl force-pushed the feat/experimental-component-api branch from 34e2df5 to 39b4a95 Compare May 2, 2021 17:37
@glennsl glennsl force-pushed the feat/experimental-component-api branch from 39b4a95 to 4b1e719 Compare May 2, 2021 17:44
@glennsl
Copy link
Contributor Author

glennsl commented May 2, 2021

There, I've moved the comp! macro to seed and added a Component trait that can eventually be expanded upon, along with a comment outlining my thoughts a bit more. I'm not entirely sure I've put them in their appropriate places though.

Comment on lines +22 to +23
#[allow(clippy::module_name_repetitions)]
pub struct ButtonComponent<Ms: 'static> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit of an unsightly wart for those who use clippy. Not entirely sure what to do about it, but probably either the trait or the naming convention here needs to be renamed.

}

impl<Ms> Component<Ms> for ButtonComponent<Ms> {
fn render(&self) -> Node<Ms> {
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between view and render? I really don't care about the name itself but I like consistent naming ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, yeah. I'm not sure what I was thinking there :) Fixed in b57c930

@glennsl
Copy link
Contributor Author

glennsl commented May 3, 2021

Some further thoughts:

  1. There's currently no way to conditionally set a property. Perhaps something like disabled => ?maybe_disabled to apply an Option would work. This makes the macro significantly more complicated though.

  2. It would be nice to have a macro provide a simpler interface for defining a component. Ideally something like:

#[component(Button)]
fn view<Ms: 'static>(label: String, disabled: Option<bool>) -> Node<Ms> {
  // ...
}

I can already see several limitations with this though, like lack of polymorphic arguments (assuming we need to store the component configuration to be able to instantiate it in local updates later). And depending on how we want to manage component state, we might want to extend the Component trait with a Model, Msg and init and update functions instead.

  1. Again assuming we need to store the component configuration for later instantiation in local updates, the component will need to own all its data. That's a relatively minor limitation though.

@mankinskin
Copy link

I have earlier wanted something like this, you might want to look at my components implementation for inspiration.

@flosse
Copy link
Member

flosse commented Jun 18, 2022

Feel free to re-open this PR but as long as #672 is not solved I think this is issue not relevant 😞

@flosse flosse closed this Jun 18, 2022
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