-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Component v2 #1646
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
Component v2 #1646
Conversation
|
@siku2 any thoughts? |
|
Hmm... The whole I'm imagining something like this: // Context contains props, state, and the component link
struct Props {
init_text: String
}
struct Foo {
counter: usize
}
impl Component for Foo {
type Properties = Props;
fn update(ctx: &mut Context, msg: Self::Message) -> bool {
match msg {
Msg::Increment => {
ctx.state.counter += 1;
true
}
}
}
fn view(ctx: &Context) -> Html {
html! { <span>{ &ctx.props.text }</span> }
}
} |
|
@siku2 yeah thanks for talking some sense into me. That's a better solution than using more magic macros, I'll switch to that 👍 |
| use yew::component::{Component, Context}; | ||
| use yew::{html, Html, Legacy, ShouldRender}; | ||
|
|
||
| type Slider = Legacy<LegacySlider>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't migrate this Slider intentionally to show how a project could slowly migrate away from the legacy component trait
|
Visit the preview URL for this PR (updated for commit 53fe13d): https://yew-rs--pr1646-yew-comp-3eimqgb8.web.app (expires Tue, 01 Dec 2020 14:39:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
siku2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it!
| use crate::html::{Html, Properties, Scope, ShouldRender}; | ||
|
|
||
| /// Context | ||
| pub type Context<T> = Scope<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these type aliases only add confusion... Is there a good reason why we don't use the same name everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point. I will just move yew::html::Scope to yew::component::Context.
| @@ -1,4 +1,4 @@ | |||
| #![allow(clippy::needless_doctest_main)] | |||
| #![allow(clippy::needless_doctest_main, deprecated)] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't #[allow(deprecated)] be applied locally so we don't accidentally end up using deprecated API of another crate in the future?
| since = "0.18.0", | ||
| note = "Please switch to the yew::component::Component trait" | ||
| )] | ||
| pub trait LegacyComponent: Sized + 'static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to LegacyComponent seems like more of a pain than just upgrading to the new Component trait. Is there anything I'm missing that would make the transition impossible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the big dilemma I had when making these changes. Ideal situation would be:
- Create new Component interface which works well with yew-macro
- Deprecate existing Component interface but have it still be compatible with yew-macro
But these two are conflicting at the macro level. I decided to prioritize the new interface experience over having a nice deprecated experience but try my best to minimize changes needed to switch to the legacy component interface.
I opted to rename to LegacyComponent to make it obvious to devs which components have been upgraded but I don't feel too strongly about that. The main annoying thing is the macro changes to wrap all the html! components with Legacy<..>. Perhaps it is too annoying. I really care about this upgrade being smooth because upgrading to the new Component is tedious.
Call me crazy, but @siku2 what do you think about adding a new macro? It would act the same as html! but would handle the new Component interface. I'm thinking it could be called vdom!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel very conflicted about this. I very much understand the need to make the transition as seamless as possible but we might be worrying a bit too much about this given that Yew is officially still at major version zero (i.e. initial development).
To quote semver.org:
If you’re worrying a lot about backwards compatibility, you should probably already be 1.0.0.
Adding a new macro is a step too far in my opinion. If you think the migration to the new component trait is too tedious, I'm fine with keeping LegacyComponent around until the following release. We can deal with the Legacy<_> wrapper by suggesting the use of a type alias type MyActualComponent = Legacy<MyComponentImplementation>.
The migration guide could look something like this:
- Replace
impl Componentwithimpl LegacyComponent - Rename your component
Footo something likeFooComplocally - Create a type alias
pub type Foo = Legacy<FooComp>; - Use the
Footype instead of theFooCompstruct
Personally though, I wouldn't even consider switching to LegacyComponent, if I have to do some modifications in each file anyway, I might as well upgrade to the new Component trait.
| route::Route, | ||
| switch::{Routable, Switch}, | ||
| }; | ||
| pub use crate::switch::Switch as Routable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we actually rename Switch -> Routable and then create a deprecated type alias for Switch.
#[deprecated(note="use `Routable` instead")]
pub type Switch = Routable;Co-authored-by: Simon <[email protected]>
Co-authored-by: Simon <[email protected]>
Co-authored-by: Teymour Aldridge <[email protected]>
|
I really do love this. I feel like the clunkiest part of writing components was the ComponentLink and Properties. Now that the component is directly managed by its own concrete type, that means less boilerplate, more fun, and also enables some other things that could be implemented in the future. |
|
I think we should just remove Also, what's left to be done in this PR? Is there any way I could help out? |
Agreed!
Feel free to take over this PR |
|
Should this now be closed in favour of #1961? |
Description
This PR aims to address two common issues with Components:
changelifecycle methodThis PR addresses the two above issues with breaking changes and renames and deprecates the existing
Componenttrait (now namedLegacyComponent) to encourage devs to switch to the new trait.Fixes #830
Changes
Componenttrait!changemethod renamed tochanged(and is only called when props != new props)Contextwhich right now is just an alias toScopeyew::html::ComponenttoLegacyComponenthtml!to only handle new components so legacy components must be wrapped inLegacy<..>Propertiesmust now implementPartialEqChecklist
cargo make pr-flow