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

Introduce unstruct macro to help destructuring #39

Closed
wants to merge 3 commits into from

Conversation

cecton
Copy link
Member

@cecton cecton commented Oct 29, 2023

I'm working with Yew in production since November 2020 and there are a lot of things I think would be great as quality of life improvements.

One of them being the introduction of "ImplicitClone". Every "Properties" struct in Yew should actually be cheap to clone. This is because during rendering the old properties struct is compare with a new one freshly created so it is important that the fields themselves are cheap to clone.

Now that we know that each field of a Properties struct is cheap to clone, we can actually also improve the built-in destructuring of Rust. Rust destructuring has 2 main problems:

  • it assumes it doesn't know if a field can taken as reference or can be dereferenced (moved for Copy types)
  • you always need to provide the type of the struct itself. I'm not sure why that is because this doesn't exist in JS and we can definitely live without it.
const { foo, bar } = props();

With this macro, we can drop the name of the struct plus we don't need to clone all the fields if we don't have too.

unstruct! {
    let { foo, bar } = ctx.props();
}

I think the macro is better fitted here in implicit-clone rather than Yew but I don't have strong opinion on that.

@kirillsemyonkin
Copy link
Collaborator

kirillsemyonkin commented Oct 29, 2023

  • Where did the need for this come from?
  • Is cloning a struct via (*s).clone() any different from cloning individual fields? I feel like having to wrap the whole statement in unstruct! is a bit clunky. How about let (x, y, z) = unstruct!(s);? (although would be problematic to know what fields there are...)

@kirillsemyonkin
Copy link
Collaborator

Actually, #[derive(Unstruct)] and let (x, y, z) = s.unstruct() sounds cool

@cecton
Copy link
Member Author

cecton commented Oct 29, 2023

Where did the need for this come from?

sorry I forgot to fill the description on the ticket, will do asap

@cecton
Copy link
Member Author

cecton commented Oct 29, 2023

  • I feel like having to wrap the whole statement in unstruct! is a bit clunky.

yeah I agree but worth the effort, I'll update the description of the PR xD

  • How about let (x, y, z) = unstruct!(s);? (although would be problematic to know what fields there are...)

yeah exactly, you need the name of the fields inside the macro

@cecton
Copy link
Member Author

cecton commented Oct 29, 2023

I feel like having to wrap the whole statement in unstruct! is a bit clunky.

I updated the ticket description.

It's true that having to write unstruct! is a bit clunky but at least it's the same word everywhere. The builtin destructuring forces you to provide the name of the Properties struct instead which is annoying. So I would say one clunkyness balance the clunkyness of the other.

The only remaining advantage of unstruct is that it clones the fields separately. In the end it's really more about development efficiency and convenience.

@kirillsemyonkin
Copy link
Collaborator

Every "Properties" struct in Yew should actually be cheap to clone

It is not forced to be ImplicitClone though (add #[derive(ImplicitClone)]?), only PartialEq is needed.

With this macro, we can drop the name of the struct

In function component world, this is done with yew-autoprops. It could become a part of Yew and a missing & would call for having an ImplicitClone on the prop.

I'm not sure why that is because this doesn't exist in JS

*Rust. This is called anonymous structs, and judging by some discussions, this has idea has been thrown out by the community multiple times, no clue why. I guess the main problem is with { a: i32, b: f32 } not possible to be the same as { b: f32, a: i32 }.

@cecton
Copy link
Member Author

cecton commented Oct 29, 2023

It is not forced to be ImplicitClone though (add #[derive(ImplicitClone)]?), only PartialEq is needed.

Yup I wanted to keep the sentence simple. It's really only the fields that need to be cheap to clone. And it's not a constraint.

I was thinking also about a "derive(ImplicitClone)" the other day to force all the fields to implement ImplicitClone. I don't know if it is really a good idea because it will break quite a lot of code for everybody if we enforce it. Also I'm not sure how to implement this: you can impl ImplicitClone on any struct, even if the fields are not cheap to clone.

In function component world, this is done with yew-autoprops. It could become a part of Yew and a missing & would call for having an ImplicitClone on the prop.

cool I didn't know! I don't use much function component though. Usually I go for function component only if there is no state. I like the full fledged components of Yew. But yeah autoprops could definitely clone by default

I'm not sure why that is because this doesn't exist in JS

*Rust.

I mean in JS it doesn't exist. In JS you don't need to specify the type of the object to destructure it.

const { foo, bar } = object;

This is called anonymous structs, and judging by some discussions, this has idea has been thrown out by the community multiple times, no clue why. I guess the main problem is with { a: i32, b: f32 } not possible to be the same as { b: f32, a: i32 }.

I guess there are probably valid reasons. But in the context of ImplicitClone and Yew the thing is just plain annoying.

For example. I often end up with multiple of them because sometimes I need dereferencing, sometimes I need cloning.

struct Props {
    some_complicated_stuff: Rc<Stuff>,
    enabled: bool,
}
fn view() {
    let Props { some_complicated_stuff, .. } = &ctx.props();
    let Props { enabled, .. } = *ctx.props();

    if enabled { // this won't work if we have a ref of bool
    // ...
}
fn update(message: Self::Message) -> bool {
    let Props { some_complicated_stuff, .. } = ctx.props().clone();
    let Props { enabled, .. } = *ctx.props();

    if enabled { // this won't work if we have a ref of bool
    // ...

@kirillsemyonkin
Copy link
Collaborator

I mean in JS it doesn't exist

I know what you mean now, the repeating-type does not exist; I meant the feature that is missing from Rust doesn't exist in Rust

I go for function component only if there is no state

I don't see much use in any component that has no state... At that point it can be a non-component fn(...) -> Html.

full fledged components of Yew

I get that you mean struct components, but function components are also full fledged, they just are missing some optimizations, but that's not very important from the user stand point - for them function components can actually do every task at hand, I personally do not see struct components as any more capable (and some Yew discussions focus on getting rid of struct components altogether), more often more cumbersome especially in the link vs hook department.

sometimes I need dereferencing, sometimes I need cloning

Not sure what you mean by those examples. Binding via * deref operator performs a Copy. Clone and Copy, when latter is available, are the same. In your view example it kind of makes sense that you want to clone everything but that Rc (although, Rc is ImplicitClone, so why not .clone() it as well?), but in your update example enable will no longer be a ref even if it was in the first destructure.

fn update(message: Self::Message) -> bool {
    let Props { some_complicated_stuff, enable } = ctx.props().clone();
    // some_complicated_stuff: Rc<Stuff>
    // enable: bool

@futursolo
Copy link
Member

I guess there are probably valid reasons. But in the context of ImplicitClone and Yew the thing is just plain annoying.

For props in Yew, you can already write the following, which I think is shorter than destructure assignment of anonymous structs.

#[function_component]
fn Comp(Props { ref field }: &Props) -> Html {
    html! {}
}

@cecton
Copy link
Member Author

cecton commented Oct 29, 2023

For props in Yew, you can already write the following, which I think is shorter than destructure assignment of anonymous structs.

Yes I like that pattern except that it gives references. I often have to extract it from the declaration like this:

#[function_component]
fn Comp(props: &Props) -> Html {
    let Props { enabled, field } = props.clone();
    html! {}
}

Booleans are especially annoying.

I personally do not see struct components as any more capable (and some Yew discussions focus on getting rid of struct components altogether), more often more cumbersome especially in the link vs hook department.

Right now, struct component exist, and I prefer to use it over hooks. It's my choice.

but in your update example enable will no longer be a ref even if it was in the first destructure.

Yes in my old code there is a lot of use of references. Nowadays I could just not use references (&), nor dereference (*) but clone all the things. Since the props are cheap to clone, the cost is acceptable for me, booleans will always work, and overall it's much simpler.

Well anyway. I think we are digressing quite a lot.

To be clear, here are my points:

  1. struct components in Yew exist and are valid and supported
  2. props (fields) should always be cheap-to-clone
  3. unstruct is probably not very useful when using function components (probably best to improve yew-autoprops if it can't handle having values instead of references)
  4. builtin destructuring is great for Rust but in the context of yew it's unnecessary burden. having this small macro that clone all the things selectively is much more productive

@ColonelThirtyTwo
Copy link
Contributor

For props in Yew, you can already write the following, which I think is shorter than destructure assignment of anonymous structs.

Yes I like that pattern except that it gives references. I often have to extract it from the declaration like this:

#[function_component]
fn Comp(props: &Props) -> Html {
    let Props { enabled, field } = props.clone();
    html! {}
}

Booleans are especially annoying.

struct Props {
    foo: bool,
    bar: String,
}

fn doit(&Props {foo, ref bar}: &Props) {
    // foo is copied, bar is a reference
}

@kirillsemyonkin
Copy link
Collaborator

kirillsemyonkin commented Nov 9, 2023

@ColonelThirtyTwo the discussion is that

  • some props need to be cloned rather than simply deref-copied or kept as a ref
  • the problem mostly occurs with struct components, as ctx.props() must be used without any alternative available
  • with fn components yew-autoprops became a part of yewstack, and that allows you to implicit_clone some of the props right there by omitting &

p.s. im not sure whether &{ref} means "keep this as &" or it means "deref and then ref"

@cecton
Copy link
Member Author

cecton commented Nov 9, 2023

struct Props {
    foo: bool,
    bar: String,
}

fn doit(&Props {foo, ref bar}: &Props) {
    // foo is copied, bar is a reference
}

I forgot about this syntax but it doesn't really matter for Yew. Not everything is copy-able primitive.

struct Props {
    foo: bool,
    bar: String,
}

fn doit(&Props {ref foo, bar}: &Props) {
    // cannot move out of a shared reference
}

@futursolo
Copy link
Member

futursolo commented Nov 9, 2023

  1. I do not think that unstruct! { let { foo, bar } = ctx.props(); } is better than let Props { enabled, field } = props.clone();. This is a custom syntax so RustFmt wouldn’t work and not necessarily easier to write due to the extra {}. In addition, the native implementation requires either a full destructure or .. which I think is a better language design, this is also not possible with a procedural macro. IDE auto completion will also stop working for adding more fields due to custom syntax.
  2. For let (x, y, z) = unstruct!(s); and similar syntaxes, it actually depends on the order of fields and not name of fields, which is not as good as the JavaScript one, resulting having to constantly checking the order of fields and error prone code. const { x, y, z } = s and const { z, y ,x } = s would ensure that x, y and z being correctly assigned to local variables. If one wants to extract a and e from a struct with 5 fields (a, b, c, d, and e), they are required to write (a, _, _, _, e) vs Props {a, e, .. }.

A short demonstration

demo-out.mp4

@cecton
Copy link
Member Author

cecton commented Nov 17, 2023

I'm closing this because I'm tired arguing + I think the syntax proposed here: #39 (comment) is actually the best. Primitives need to be dereferenced, not all types that are implicitclone.

I have updated the doc on Yew accordingly to show more of this syntax.

I admit I was wrong but I honestly think the attitude of some people here is pretty bad and I am not sure I will want to continue contributing in the future. OSS development is getting tiresome for me.

@cecton cecton closed this Nov 17, 2023
@cecton cecton deleted the unstruct branch November 17, 2023 08:28
@kirillsemyonkin
Copy link
Collaborator

I admit I was wrong

I do not think you were wrong in general, I do not know about the others, but personally I was just trying to see if there is any way we can improve it so that experience is the best.

Primitives need to be dereferenced, not all types that are implicitclone.

It would be nice to have really, but I think others shown that the way rustfmt and other tools work limit how we can do this properly.

Also haven't looked at this syntax that just hit my head:

#[unstruct]
pub fn example(#[unstruct] Example1 { a, ref b, ... }: &Example1) {
    ...
    #[unstruct] let Example2 { a, ref b, ... } = example2;
    ...
}

This looks cumbersome at first, since it is Rust double-struct-name syntax + extra #[unstruct] bits, but it does have that benefit of saving a let a = ImplicitClone::implicit_clone(a); line and also is Rust-syntax-compliant.

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.

None yet

4 participants