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

Add shorthand syntax for props #1970

Merged
merged 9 commits into from
Jul 31, 2021
Merged

Conversation

Xavientois
Copy link
Contributor

Description

Now that braces are required around props, we can allow the svelte-style shorthand syntax for props:
Instead of my_prop={my_prop}, this allows you to just write {my_prop}.

Mentioned in #1769

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

github-actions bot commented Jul 25, 2021

Visit the preview URL for this PR (updated for commit 5cec835):

https://yew-rs--pr1970-shortened-prop-synta-so64qc2v.web.app

(expires Fri, 06 Aug 2021 16:38:28 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Contributor

@mc1098 mc1098 left a comment

Choose a reason for hiding this comment

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

Pretty cool change 👍 funny this is referenced as similar to svelte by others too and yet my first thought was this aligned well because Rust does this with structs:

struct Example {
    value: &'static str,
}
// .. 
let value = "Hello, world!";

let my_example = Example {
    value
};

Specifying value: value will make clippy unhappy.
So I think this syntax should not feel too foreign for rustaceans either 🦀.

I haven't looked through the documentation or examples fully to remove the old syntax because I don't think it's a bad thing for users to choose the explicit style so I'm happy if some sneak through the net, might even argue that we shouldn't remove all instances of the explicit style.

Apart from my comments below, I think we should add specific failure cases that cause the new errors for trybuild. I'm happy the errors work correctly but still think we should add it to trybuild tests for the future.

Copy link
Contributor

@mc1098 mc1098 left a comment

Choose a reason for hiding this comment

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

I'm happy with the changes now the tests have been added to check the new errors :)

To reiterate the point I made earlier, I haven't checked if every instance the shorthand could be used; it is used as I think it's acceptable to use the more explicit style too.

@siku2 (sorry for the ping 👀) if you'd prefer the shorthand is always used now for examples/docs then I'm happy to review this PR again with that in mind.

@Xavientois
Copy link
Contributor Author

To reiterate the point I made earlier, I haven't checked if every instance the shorthand could be used; it is used as I think it's acceptable to use the more explicit style too.

I think it's fine for users to choose which syntax to use, so I don't think we should enforce usage of this shorthand, but I used a regex search to match all instances of the explicit syntax in order to change it, so I don't believe there are instances of it left in the documentation.

@siku2
Copy link
Member

siku2 commented Jul 30, 2021

I think it's fine for users to choose which syntax to use, so I don't think we should enforce usage of this shorthand

I feel like it should emit a warning much like clippy::redundant_field_names in the future. Since this isn't well-supported by macros yet, I don't particularly mind if there are a few inconsistencies with regards to the syntax in the documentation or examples.

@Xavientois
Copy link
Contributor Author

I feel like it should emit a warning much like clippy::redundant_field_names in the future. Since this isn't well-supported by macros yet, I don't particularly mind if there are a few inconsistencies with regards to the syntax in the documentation or examples.

I have created an issue to address this later #1979.

Is there anything else that needs to be done in this PR?

@siku2
Copy link
Member

siku2 commented Jul 31, 2021

I have created an issue to address this later #1979.

Great, thanks!

Is there anything else that needs to be done in this PR?

Nope!

@siku2 siku2 merged commit c1226a2 into yewstack:master Jul 31, 2021
@voidpumpkin voidpumpkin added A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate A-yew-router Area: The yew-router crate labels Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate A-yew-router Area: The yew-router crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants