yew
yew copied to clipboard
yewstack
Can't access old props in `changed()` to compare to new props
Problem
In Yew 0.18, since properties were stored in the component, it was trivial to compare the old properties to the new ones in the change() lifecycle method. In Yew 0.19 this is no longer possible, as the properties are now stored in Context and were already updated by the time changed() is called. It looks like the only way I can do this is to copy the properties back into the model object after comparing them in changed().
I need this because I'm trying to determine whether to overwrite potential user changes to editable fields and to do that I need to check if individual relevant properties changed rather than the whole struct. More generally, this would also help when deriving the component state from properties is expensive, so only the property fields that changed need to be applied to the state.
Expected behavior
A simple solution would be to swap the old properties with the new ones instead of overwriting them and then pass the old properties as a new argument to changed(). This way I can easily compare them. This is of course a breaking change.
Environment:
- Yew version: 0.19.3
Questionnaire
- [ ] I'm interested in fixing this myself but don't know where to start
- [ ] I would like to fix and I have a solution
- [ ] I don't have time to fix this right now, but maybe later
One way to do this would be to store the previous props in your component struct and compare it with the new props in changed method.
If I recall correctly, it is possible to pass the old props in the changed method but doing it now would be a breaking change. It would also mean that old props will live longer, instead of being dropped and the memory being returned to allocator for later use
Storing the previous props is what I've done, but it's annoying because I have to remember that this is supposed to be immutable outside of changed(), and it doubles memory usage.
It would also mean that old props will live longer, instead of being dropped and the memory being returned to allocator for later use
Only just barely. The code that calls changed() already has both the old and the new props, it just replaces the old with the new right before invoking changed(). If the old props were passed to changed() that would just extend the lifetime of the old props very slightly, for the duration of the changed() call, but that should be very fast and they'd be dropped immediately afterward.
The very slight lifetime increase of the old properties is such a minor thing, and is definitely worth it to not have to store a clone of the properties in the component state.
If I recall correctly, it is possible to pass the old props in the changed method but doing it now would be a breaking change.
Sure, which means it should be done for v0.20 (which I see it's not currently in the milestone for).
I could also imagine a hack to add it today, adding a third old_props: Option<Rc<COMP::Properties>> field to Context and an old_props(&self) method that returns either the old_props if set, or the props otherwise (to avoid it being optional). Then this property can be set only for the duration of the call to changed(). That is however a bit of an odd API, possibly worth it if v0.20 is a long ways off but overall it's probably better to just change the API for changed() in v0.20.
I agree that changing the signature of changed is a simple change to the API that can probably be upgraded by a bit of regex matching. How many components actually define changed in the first place instead of relying on the default impl?
Yes I think it's fine doing a bit of breaking changes. We're on a phase of trial and error so there are a few things here and there we need to improve for the greater good. It's a good thing for 0.20 imo
Small regex already. Maybe we should put that in the release changelog?
perl -p -i -e 's/fn changed\(&mut self, (\w+): &Context<Self>\)/fn changed(&mut self, $1: &Context<Self>, _old_props: &Self::Properties)/g' $(find . -name \*.rs)
Instead of changing the signature and returning old_props, maybe we can modify ctx.props() to return Rc<COMP::Properties>?
This should suffice cases where users would like to store a previous props without cloning and would be a much smaller change.
@futursolo yes I just noticed there is an Rc in there nowadays!
I started with Self::Properties in signature but I changed it to &Self::Properties because it's also easier to implement. This might be the best of both worlds?
ctx.props() returning 2 different properties isn't handy. I have 2 arguments against it:
- I'm currently using 0.19 for a customer and there are a lot of
ctx.props().somethingeverywhere. I expect other people will have the same issue. - ~~I think overriding
fn changed()is mostly done to do props management between old and new props. It does make sense to have it in the signature directly so we don't put the burden on Context.~~ [edit: it's also done when you want to update the state after the props have changed]
Let me know what you think
ctx.props() returning 2 different properties isn't handy. I have 2 arguments against it:
I was suggesting to change fn props(&self) -> &Self::Properties to fn props(&self) -> Rc<Self::Properties>.
So if one wants to store old props, they can:
struct Comp {
old_props: Rc<Comp::Properties>
}
impl Component for Comp {
// ...
fn create(ctx: &Context<Self>) -> Self {
Self { old_props: ctx.props().clone() }
}
fn changed(&mut self, ctx: &Context<Self>) -> bool {
let should_render = self.old_props.field_a == ctx.props().field_a;
self.old_props = ctx.props().clone();
should_render
}
}
Ah I see! Thanks for the example
I'm personally not super fan of this...
It exposes the fact that we use Rc for storing props. Before making the PR I didn't even realize that. Maybe this is something that can change in the future and we don't want people to have Rc<Props> in their code.
It also pushes the logic of storing old props to the user. It's not necessarily bad but I think comparing old/new props is a common pattern in those kind of frameworks.
It exposes the fact that we use Rc for storing props. Before making the PR I didn't even realize that. Maybe this is something that can change in the future and we don't want people to have
Rc<Props>in their code.
I agree with that, I'd also not expose that there is an Rc behind the scenes that easily. I can't quite remember, but is it an Rc only so it can be copied in the VDom without needing Comp::Properties: Clone? Could it be changed into a Box with the change of splitting dom_bundle from vdom?
In any case, the proposed change also exposes that in fact both set of properties are accessible at the same time. If we'd find a way to have borrowed properties (with lifetimes - to avoid clones), I don't think this will actually hold up. But for the moment I think it's fine.
I very much don't want to have to store the old properties, and I'm not coming up with any benefit of returning Rc<Props> outside of "let me store the old properties". My issue with having to store the properties today isn't so much "cloning is expensive" it's the fact that I'm having to put a copy of the properties into all of my components and remembering to compare and update it in changed().
In any case, the proposed change also exposes that in fact both set of properties are accessible at the same time. If we'd find a way to have borrowed properties (with lifetimes - to avoid clones), I don't think this will actually hold up. But for the moment I think it's fine.
Both sets of properties must be accessible at the same time in order to compare them, which Yew does automatically but even if it didn't it's pretty important to be able to compare them in order to avoid re-rendering just because the parent component re-rendered even though the properties didn't change.
In my proposal in #2851 I'm using immutable reference. Does that sound good?
fn changed(&mut self, ctx: &Context<Self>, old_props: &Self::Properties) -> bool {