Merge port of yew-autoprops in Yew to yew-autoprops #10

cecton · 2023-11-05T09:07:33Z

Fixes #8
Fixes #7
Fixes #9
Fixes #3
Related to yewstack/yew#3505

cecton · 2023-11-05T09:22:48Z

kirillsemyonkin · 2023-11-05T09:41:56Z

Seems like its generating an unexpected comma somewhere (be it struct or fn) when there are no props but there are generic types

kirillsemyonkin · 2023-11-05T09:46:11Z

Repro:

#[autoprops]
#[function_component]
pub fn Test<T>() -> Html {
    html! {}
}

cecton · 2023-11-05T09:59:05Z

@kirillsemyonkin fixed

kirillsemyonkin · 2023-11-05T09:59:40Z

I think to trigger CI you may wanted to rerun all jobs, sometimes I see that button in the actions section

cecton · 2023-11-05T10:00:11Z

No I really wanted to trigger the CI with a different commit hash ^_^

cecton · 2023-11-05T10:01:15Z

TIL that you need to add the rust version in swatinem/cache key for it to work properly 🤦‍♀️ I have like 10 repos that are not doing that huh

kirillsemyonkin · 2023-11-05T10:06:41Z

Here is another one

cecton · 2023-11-05T10:18:41Z

Fixed

kirillsemyonkin · 2023-11-05T10:35:33Z

Autoprops does not require -> Html on the fn, nor HtmlResult works (seems like that is replaced by Html) - following does not work:

#[autoprops]
#[function_component]
pub fn Test() -> HtmlResult {
    Ok(html! {})
}

Defaults for fn generic type params are ignored by Rust, here are three cases (doesnt work, good; works, good; doesnt work, oops):

cecton · 2023-11-05T11:24:30Z

@kirillsemyonkin fixed

kirillsemyonkin

Works now for my entire project on every single component (10,280 lines of Yew code) - empty props, generics, non-cloning, auto cloning, public, private, etc etc

ranile

Looks mostly good, just a couple of things that need addressing

src/autoprops.rs

Related to yewstack/yew-autoprops#10

@kirillsemyonkin

Following the advice of @kirillsemyonkin, I'm adding a function `implicit_clone()` which will enforce that the type is implementing `ImplicitClone` while still actually cloning. Related to yewstack/yew-autoprops#10 (comment)

cecton · 2023-11-09T14:18:30Z

@hamza1311 @kirillsemyonkin ok I think this is ready now

src/autoprops.rs

kirillsemyonkin · 2023-11-12T11:50:32Z

What is the current status of this?

futursolo · 2023-11-12T13:29:23Z

tests/function_component_attr/autoprops-fail.stderr

+7 | fn CantAcceptReceiver(&self, b: bool) -> Html {
+  |                       ^^^^^
+
+error: could not find #[function_component] attribute in function declaration (#[autoprops] must be placed *before* #[function_component])


I think we cannot enforce a function_component macro by reading the macro name as function_component can be renamed with as and can be implemented via other macros like stylist::yew::styled_component.

Done in e8fe2f0

Though imo we should use a separate proc macro instead of outputting the function component from an extended macro. This allows better composition:

#[styled_component] #[autoprops] #[function_component] fn MyStyledComponent() -> Html { html! {<div class={css!("color: red;")}>{"Hello World!"}</div>} }

yeah #[styled] #[autoprops] #[component] / #[autoprops] #[styled] #[component] (yewstack/yew/#3447 would be a breaking change btw so stylist would need to be updated anyway) should compose well, but its up to @futursolo to make a breaking change just for that

src/autoprops.rs

futursolo · 2023-11-12T13:54:45Z

tests/function_attr_test.rs

+#[test]
+fn tests_pass() {
+    let t = trybuild::TestCases::new();
+    t.pass("tests/function_component_attr/*-pass.rs");


Pass tests should also only be run with MSRV.
Rustc may introduce additional lints over the time to fail pass tests.

Tests breaking somehow to me sounds like a good idea, despite being slightly annoying upon trying to perform an unrelated PR (already happened with clippy before). Maybe have them run under both MSRV and latest stable so we can see whether such a lint change occured?

@kirillsemyonkin it's already what I did here

But in Yew we only run them against MSRV.

imo it's best to run it on both because we need the thing to keep working on stable and to be alerted asap when it's not.

I don't mind restricting this test to MSRV though.

@futursolo do you agree with the argument or you still prefer the passing test to run only on MSRV?

cecton · 2023-11-17T07:49:40Z

@yewstack/yew can I have a final review on this?

Madoshakalaka · 2023-12-03T13:25:44Z

@cecton hi you are my angel, thanks for the work

kirillsemyonkin · 2023-12-03T14:19:04Z

@yewstack/yew can I have a final review on this?

Can we skip such a review since it is more-or-less separate from Yew and proceed to merge? We can have extra merges later, independent of Yew's slow cycle

kirillsemyonkin · 2023-12-03T14:20:59Z

Ideally anyone's work on a stylist PR for #[styled_component] -> #[styled] (for #[styled] #[autoprops] #[component] progress) should happen somewhere around now as well

cecton · 2023-12-04T12:35:31Z

Can we skip such a review since it is more-or-less separate from Yew and proceed to merge? We can have extra merges later, independent of Yew's slow cycle

Yeah I think the other maintainers are a bit overbooked at the moment so let's merge it as is. Worst case scenario we open issues or fix things later on.

stalling

cecton added 4 commits November 5, 2023 10:06

Merge port of yew-autopros in Yew to yew-autoprops

859672a

Remove useless step in the workflow

c3e88fa

Add caching in the CI workflow

3af6236

Force adding Cargo.lock

a1c7fb3

cecton requested review from futursolo and ranile November 5, 2023 09:22

cecton added 3 commits November 5, 2023 10:25

Forgot to update README.md

0fc2002

Make passing test run on any version of Rust

388d018

Add cache key on Rust version to differentiate both jobs

f2995ed

cecton added 2 commits November 5, 2023 10:54

Trigger CI

15269d4

Fix issue with single generic and add test

7ff4375

Fix for private fields & add test

cd00065

Also add failing test for private components

27427cc

cecton added 2 commits November 5, 2023 11:54

Preserve return type

e64e116

Fix generics with default not working

00718cd

cecton requested a review from kirillsemyonkin November 5, 2023 11:24

Just making sure the where_clause is not pushed unexpectedly

b60e5cc

kirillsemyonkin approved these changes Nov 5, 2023

View reviewed changes

ranile previously requested changes Nov 5, 2023

View reviewed changes

src/autoprops.rs Outdated Show resolved Hide resolved

src/autoprops.rs Show resolved Hide resolved

src/autoprops.rs Outdated Show resolved Hide resolved

src/autoprops.rs Outdated Show resolved Hide resolved

cecton mentioned this pull request Nov 9, 2023

Add method implicit_clone() to help ensure the type is ImplicitClone yewstack/implicit-clone#44

Merged

cecton marked this pull request as draft November 9, 2023 10:28

cecton added a commit to yewstack/implicit-clone that referenced this pull request Nov 9, 2023

Impl ImplicitClone for arrays and ref arrays (#42)

2606c0f

Related to yewstack/yew-autoprops#10

Ensure cloned types impl ImplicitClone

45682cc

cecton requested a review from ranile November 9, 2023 14:18

cecton marked this pull request as ready for review November 9, 2023 14:18

futursolo reviewed Nov 9, 2023

View reviewed changes

src/autoprops.rs Outdated Show resolved Hide resolved

cecton added 2 commits November 9, 2023 17:04

It doesn't look like I need this

686f009

Use ImplicitClone re-export from Yew

ad7c72a

cecton requested a review from futursolo November 10, 2023 08:58

kirillsemyonkin mentioned this pull request Nov 12, 2023

Port yew-autoprops to yew-macro yewstack/yew#3505

Merged

2 tasks

futursolo reviewed Nov 12, 2023

View reviewed changes

src/autoprops.rs Outdated Show resolved Hide resolved

futursolo reviewed Nov 12, 2023

View reviewed changes

src/autoprops.rs Show resolved Hide resolved

futursolo reviewed Nov 12, 2023

View reviewed changes

cecton added 2 commits November 13, 2023 08:52

Rename field phantom to __yew_autoprops_phantom

e323bee

Remove check for #[function_component]

e8fe2f0

cecton mentioned this pull request Nov 13, 2023

Attributes of function arguments should be applied to the function or to properties? #11

Open

cecton requested a review from futursolo November 15, 2023 17:39

cecton merged commit 8c3bc87 into yewstack:main Dec 4, 2023
2 checks passed

cecton deleted the merge-yew-port-autoprops-to-here branch December 4, 2023 12:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge port of yew-autoprops in Yew to yew-autoprops #10

Merge port of yew-autoprops in Yew to yew-autoprops #10

cecton commented Nov 5, 2023 •

edited

Loading

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin left a comment

ranile left a comment

cecton commented Nov 9, 2023

kirillsemyonkin commented Nov 12, 2023

futursolo Nov 12, 2023

cecton Nov 13, 2023

cecton Nov 13, 2023

kirillsemyonkin Nov 13, 2023

futursolo Nov 12, 2023

kirillsemyonkin Nov 12, 2023

cecton Nov 13, 2023

cecton Nov 13, 2023

cecton commented Nov 17, 2023

Madoshakalaka commented Dec 3, 2023

kirillsemyonkin commented Dec 3, 2023

kirillsemyonkin commented Dec 3, 2023

cecton commented Dec 4, 2023

Merge port of yew-autoprops in Yew to yew-autoprops #10

Merge port of yew-autoprops in Yew to yew-autoprops #10

Conversation

cecton commented Nov 5, 2023 • edited Loading

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin commented Nov 5, 2023

cecton commented Nov 5, 2023

kirillsemyonkin left a comment

Choose a reason for hiding this comment

ranile left a comment

Choose a reason for hiding this comment

cecton commented Nov 9, 2023

kirillsemyonkin commented Nov 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Nov 17, 2023

Madoshakalaka commented Dec 3, 2023

kirillsemyonkin commented Dec 3, 2023

kirillsemyonkin commented Dec 3, 2023

cecton commented Dec 4, 2023

cecton commented Nov 5, 2023 •

edited

Loading