Components v2 (2) #1961
Components v2 (2) #1961
Conversation
|
The API is mostly in it's final form. I would prefer if anyone, especially maintainers, reviewed it before I go ahead and fix the documentation and examples as that would make the PR bigger, harder review and change. There are a couple of thing I would note:
PS: This PR is marked as draft because it isn't ready to be merged yet. |
|
Visit the preview URL for this PR (updated for commit 4c188eb): https://yew-rs--pr1961-comp-rewrite-vj5zauo3.web.app (expires Tue, 31 Aug 2021 16:36:57 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
|
This PR is ready for review and I'm marking it as such. |
There was a problem hiding this comment.
Website Documentation that mentions ComponentLink, neq_assign, or uses the old syntax of the Component trait:
- docs/advanced-topics/optimizations.md
- #neq_assign
No longer needed - #using smart pointers effectively
refers toComponent::change - #Pure Components
mentionsneq_assign
- #neq_assign
- docs/concepts/components/callbacks.md
- #Callbacks
examples useself.link.[callback_fn]
- #Callbacks
- docs/concepts/components/children.md
- #General usage
example stores props inComponentstruct +view(&self) - #Advanced usage
As above (A/A) - #Enum typed children
A/A - #Optional typed child
A/A
- #General usage
- docs/concepts/components/properties.md
- #Derive macro
Propertiesmust implementPartialEqinstead ofClone
- #Derive macro
- docs/concepts/function-components/attribute.md
- Initial section
No need to explicitly state that the parameter needs to implementPropertiesandPartialEq
"The parameter type needs to be a reference to a type which implementsPropertiesandPartialEq(ex.props: &MyProps)." Can remove thePartialEq? as allPropertiesmust bePartialEq. - #Example
Example props deriveClone - #Generic function components
Example props andTderiveClone
- Initial section
- docs/concepts/html/classes.md
- #Components that accept classes
example stores props inComponentstruct +view(&self)
- #Components that accept classes
- docs/concepts/html/components.md
- #Nested
examples stores props inComponentstruct +view(&self) - #Nested Children with Props
example stores props inComponentstruct +view(&self)
- #Nested
- docs/concepts/html/elements.md
- #DOM nodes
example uses old view syntaxview(&self) - #Listeners
examples usesComponentLink+ oldComponentsyntax
- #DOM nodes
- docs/concepts/html/list.md
- #Iterators
example usesself.propssyntax
- #Iterators
- docs/concepts/components.md
- #Create
refers toComponentLinkand example uses old syntax - #View
example uses old view syntax - #Rendered
example uses old syntax - #Update
example uses old update syntax - #Change
whole section needs updating - #Associated Types
Last paragraph should probably mention that properties are passed throughContext?
- #Create
- docs/concepts/contexts.md
- #Struct components
refers toComponentLinkand example uses old syntax
- #Struct components
- docs/more/development-tips.md
- #Jetbrains IDEs
template using old syntax - #VS Code
A/A
- #Jetbrains IDEs
| impl<COMP: Component> Context<COMP> { | ||
| /// The component scope | ||
| #[inline] | ||
| pub fn link(&self) -> &Scope<COMP> { |
There was a problem hiding this comment.
I think to a new user this might seem odd, calling link and getting back a Scope 🤔 Not sure if it's worth having an alias again or whether just expanding on that doc comment is enough. It might be that I'm splitting hairs about something that will always be a bit vague to a new user anyway.
It made me pause so thought I'd just add this note here in case.
There was a problem hiding this comment.
I'm not sure if this should be renamed. I thought the same while renaming but left it that way. I think that type aliases when they point to the same thing, under a different name only do nothing but cause confusion/indirection.
| /// ``` | ||
| ///# use yew::{Html, Component, Properties, ComponentLink, html}; | ||
| ///# struct Model; | ||
| ///# #[derive(Clone, Properties)] | ||
| ///# struct Props { | ||
| ///# prop: String, | ||
| ///# } | ||
| ///# impl Component for Model { | ||
| ///# type Message = ();type Properties = Props; | ||
| ///# fn create(props: Self::Properties,link: ComponentLink<Self>) -> Self {unimplemented!()} | ||
| ///# fn update(&mut self,msg: Self::Message) -> bool {unimplemented!()} | ||
| ///# fn change(&mut self, _: Self::Properties) -> bool {unimplemented!()} | ||
| ///# fn view(&self) -> Html { | ||
| /// html! { | ||
| /// <Model prop="value" /> | ||
| /// } | ||
| ///# }} | ||
| /// ``` |
There was a problem hiding this comment.
I like the idea of having doc examples for the Component trait's associated types, however, I don't think the examples removed were particularly useful so I'm in favour of removing them here and letting another PR cover adding more useful ones.
|
@mc1098 I made the requested changes, please take a look again. PS: thanks for taking the time to look through the docs that I missed to update. |
|
@mc1098, hopefully that should be all. I'm also working on a tutorial, which should complement the docs (though it uses function components). I think we should update the docs to be more precise after that. |
Yep, enhancing the docs should be different PR(s) and I think this PR should just correct the docs relevant to it's own changes, which I think is now done 🎉.
I'm going to have a look at this, need a better understanding of this part of the code anyway :D |
I ended up fixing those myself. I had to store the children prop inside the component but it works. I'm not sure if that's the best way to handle this though but that's how it was before. Context optimizations are outside the scope of this PR so I guess they can come in later. |
|
I think this might hit on a common use case though - a I wonder whether the fn changed(&mut self, _old_props: &Self::Properties, _ctx: &Context<Self>) -> ShouldRenderI think this should be possible since we have access to the old props within the update event so we could just do something like this: UpdateEvent::Properties(mut props, node_ref, next_sibling) => {
// When components are updated, a new node ref could have been passed in
state.node_ref = node_ref;
// When components are updated, their siblings were likely also updated
state.next_sibling = next_sibling;
// Only trigger changed if props were changed
if state.context.props != props {
std::mem::swap(&mut state.context.props, &mut props);
state.component.changed(props.as_ref(), &state.context)
} else {
false
}
}If you agree... (looks at the documentation 👀) then I'm sorry 🙏. |
I agree, partially. There are cases where that's needed but there are also cases (see one of them, user might want to only make the call if the properties change) where it's needed outside of |
Do you mean I'm not sure I follow your point with the example given, that seems to use the current props
The only reason I think passing down the old props would be useful is because we seem to have it on hand in that update event and we can pass a reference1 to it when calling 1 I don't mind if we pass the Part of my thinking is that if the change was made then you wouldn't need to hold fn changed(&mut self, old_props: &ContextProviderProps<T>, ctx: &Context<Self>) -> bool {
let props = ctx.props();
if self.context != props.context {
self.context = props.context.clone();
self.notify_consumers();
}
old_props.children != props.children
}and |
Yes
I wasn't clear enough, consider the following; fn rendered(&mut self, ctx: &Context<Self> first_render: bool) {
let element = self.node_ref.cast::<TextField>().unwrap();
if ctx.props().field_type != self.props.field_type {
element.set_type(&JsValue::from(
self.props.field_type.to_cow_string().as_ref(),
));
}
// ...If there's
Agreed. But moving it inside |
I see thank you :)
My proposal came at no cost - we have the old/last props available at the time of calling It might be that this use case is just going to be so common place that the convenience of having last_props outweighs any potential memory cost or it might be even with your clarification I'm missing a greater point here? |
|
I think this will close #1560 too :) |
| /// Components handle messages in their `update` method and commonly use this method | ||
| /// to update their state and (optionally) re-render themselves. | ||
| fn update(&mut self, msg: Self::Message) -> ShouldRender; | ||
| fn update(&mut self, _ctx: &Context<Self>, _msg: Self::Message) -> ShouldRender { |
There was a problem hiding this comment.
Small nit:
When you use IDE auto completion for trait methods in an impl block, it will copy this exact line with the leading underscores in the argument names. I'd remove the underscores and add #[allow(unused_variables)] instead, so you don't have to remove them each time after auto completion.
Same for the other methods, of course.
There was a problem hiding this comment.
Also, as this is a good opportunity, do you (and other contributors) think that ShouldRender should be kept as an alias? I don't see much benefit in it over a comment on what the returned bool means. Furthermore, it has been rather annoying to either import another symbol or replace it with a bool after auto completion.
There was a problem hiding this comment.
Also, as this is a good opportunity, do you (and other contributors) think that
ShouldRendershould be kept as an alias? I don't see much benefit in it over a comment on what the returnedboolmeans.
I have no strong feelings either way. ShouldRender is a pretty nice alias, as alias go, but a comment on what the returned bool means seems good to me.
Furthermore, it has been rather annoying to either import another symbol or replace it with a bool after auto completion.
I guess I have gotten so used to flooding my namespace with yew::prelude::* to not have this issue :)
I'd vote to remove the alias then, as you (and others) probably hit this issue several times, whereas not knowing what the bool means in update and changed is generally only going to happen once and is easy to resolve (look at the comment).
This might be from my limited experience, but most times you don't care how the props changed and just want to rerender, if they did. On the other hand, now that checking for prop change is done by the framework, hardly anyone would need to override |
We could leave it as is, and see if a compelling use case comes up to justify this change? It's pretty easy to add (when not tangled in an already large PR :D) and as @bakape said most users are unlikely to override Once Clippy is happy then this PR looks in good shape for merging 🎉 |
I agree.
While that is true, I think it would be a good idea to reduce the amount breaking changes in future releases.
I didn't realize clippy was complaining. I'll fix that soon. |
Mostly in jest :D but I think before 1.0.0 breaking changes should be expected and is worth it for correctness, ergonomics and performance improvements. |
There was a problem hiding this comment.
Looks good to me 🎉
I think the bits about variable prefixes and the ShouldRender type can come in another PR; they are style choices and don't directly apply to these changes as they apply to Components V1 too.
Allows this PR to get merged in (if the maintainers are happy ofc), as it's big and conflict happy 🙃
# Conflicts: # examples/boids/src/slider.rs # examples/crm/src/add_client.rs # examples/file_upload/src/main.rs # examples/js_callback/src/main.rs # examples/keyed_list/src/main.rs # examples/mount_point/src/main.rs # examples/store/src/text_input.rs # examples/todomvc/src/main.rs # packages/yew-macro/tests/derive_props/fail.stderr
|
The conflicts are resolved and CI is green |
Not yet fully functional, due to yewdux not having upgraded to yewstack/yew#1961. Specifically, some examples are not working, but the stylist packages are fine.
voidpumpkin
added
A-yew
| fn update(&mut self, _ctx: &Context<Self>, _msg: Self::Message) -> ShouldRender { | ||
| false | ||
| } |
There was a problem hiding this comment.
This is so unfortunate! 😅 @hamza1311 why did you put false here by default?
If you don't mind I will make a PR to change this to true for the next release. Is there any particular reasoning for why you used false here?
Putting this to true is very handy because it allows a basic component that only needs to refresh with a callback to refresh without having to define the fn update() method.
Description
Fixes #830
Closes #1646 (supersedes it)
Closes #1560 (removes NeqAssign)
Changes
ComponentLink-Scopeis used directlyContext- passed to all component methodschangemethod is renamed tochangedchangedis only called on prop changesPropertiesno longer requireCloneto be implemented.Propertiesmust now implementPartialEqRcfor props instead of cloningChecklist
cargo make pr-flow