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

Loops - avoid to trigger "update" on the parent if the events are set on the child tag #1319

Closed
yuretz opened this Issue Nov 7, 2015 · 21 comments

Comments

Projects
None yet
5 participants
@yuretz

yuretz commented Nov 7, 2015

Trying to upgrade my app to the new riot 2.3.0 I've stumbled upon this issue. Please take a look at the following two demos.
2.2.4 demo (works)
2.3.0 demo (fails)

I haven't spent too much time debugging the problem, but the reason seems to be that the latest version of riot is always re-rendering the whole loop when focus event is fired for one of the looped items, so the original element that should actually receive focus, is deleted, and the new one is inserted even though the opts were not changed.

There are some workarounds involving the usage of e.preventUpdate in the event handler, or making sure the looped object reference stays the same if its content isn't changed, but it feels a bit wrong to me anyway.

Is it a bug or am I just not doing it right?
Thanks in advance for your answer.

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 7, 2015

Just one more thing, adding no-reorder seems to also fix the issue

yuretz commented Nov 7, 2015

Just one more thing, adding no-reorder seems to also fix the issue

@GianlucaGuarini GianlucaGuarini added bug and removed bug labels Nov 7, 2015

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 7, 2015

Member

@yuretz this is not a bug. On any update you reset the items array forcing riot rerendering all the children. This should be the right way to render your elements http://jsfiddle.net/gianlucaguarini/hr4untsz/12/

Member

GianlucaGuarini commented Nov 7, 2015

@yuretz this is not a bug. On any update you reset the items array forcing riot rerendering all the children. This should be the right way to render your elements http://jsfiddle.net/gianlucaguarini/hr4untsz/12/

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 7, 2015

OK, thanks, I see. It just feels weird, that the element is losing part of its state while being re-rendered with the same options.

BTW, just curious, why the whole loop is updated on any event in the looped item? It just goes in the opposite direction of the riot's normal "update in the parent triggers update of all children" scheme, so there might be some good reason for that.

yuretz commented Nov 7, 2015

OK, thanks, I see. It just feels weird, that the element is losing part of its state while being re-rendered with the same options.

BTW, just curious, why the whole loop is updated on any event in the looped item? It just goes in the opposite direction of the riot's normal "update in the parent triggers update of all children" scheme, so there might be some good reason for that.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 7, 2015

Member

OK, thanks, I see. It just feels weird, that the element is losing part of its state while being re-rendered with the same options.

This happens not because of riot it's how the browser works. If you remove tags from the DOM replacing them with new html the old tags state gets lost

BTW, just curious, why the whole loop is updated on any event in the looped item? It just goes in the opposite direction of the riot's normal "update in the parent triggers update of all children" scheme, so there might be some good reason for that.

Imagine the following case:

<tag>
  <ul>
     <li onclick={ remove } each={ name in items }>{ name }</li>
  </ul>
  this.items = ['one', 'two', 'three']
  remove() {
    this.items.splice(0, 1)
  }
</tag>

With the remove event we remove the unnecessary lis from the tag, but this event should be triggered updating the parent not the li itself. If you don't want to propagate the events to the parent you must use e.preventUpdate = true

Member

GianlucaGuarini commented Nov 7, 2015

OK, thanks, I see. It just feels weird, that the element is losing part of its state while being re-rendered with the same options.

This happens not because of riot it's how the browser works. If you remove tags from the DOM replacing them with new html the old tags state gets lost

BTW, just curious, why the whole loop is updated on any event in the looped item? It just goes in the opposite direction of the riot's normal "update in the parent triggers update of all children" scheme, so there might be some good reason for that.

Imagine the following case:

<tag>
  <ul>
     <li onclick={ remove } each={ name in items }>{ name }</li>
  </ul>
  this.items = ['one', 'two', 'three']
  remove() {
    this.items.splice(0, 1)
  }
</tag>

With the remove event we remove the unnecessary lis from the tag, but this event should be triggered updating the parent not the li itself. If you don't want to propagate the events to the parent you must use e.preventUpdate = true

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 7, 2015

Well, I understand that if you delete the old element and insert the new one, the old element with its state is gone. It was just somewhat unexpected for me that riot is gonna do this even if the element opts are the same.
And if instead of looping over the array of objects I loop over the array of strings, the behavior is different, and my focus demo works with the latest riot and without any workarounds. See http://jsfiddle.net/yuretz/xcu2wue4/ This is not too consistent.

In your example case, the click handler is located in the parent and it's pretty logical and convenient that it should update the parent tag automatically. But if we move the handler to a the separate child tag, we would have had to modify the parent's state more explicitly, like

this.parent.items.splice(0, 1);

And then explicit call to parent's update() function would look quite logical, because we're directly modifying the other tag's state, so we need to somehow let it know about this, if we want it to render properly. I understand that currently riot is trying to do that for me, but because it doesn't have all the information required to know whether the parent state was changed or not, you decided to do this implicit update() on every event. This is of course a possible solution, it was just not that obvious to me.

Anyway, thanks for your time and answers.

yuretz commented Nov 7, 2015

Well, I understand that if you delete the old element and insert the new one, the old element with its state is gone. It was just somewhat unexpected for me that riot is gonna do this even if the element opts are the same.
And if instead of looping over the array of objects I loop over the array of strings, the behavior is different, and my focus demo works with the latest riot and without any workarounds. See http://jsfiddle.net/yuretz/xcu2wue4/ This is not too consistent.

In your example case, the click handler is located in the parent and it's pretty logical and convenient that it should update the parent tag automatically. But if we move the handler to a the separate child tag, we would have had to modify the parent's state more explicitly, like

this.parent.items.splice(0, 1);

And then explicit call to parent's update() function would look quite logical, because we're directly modifying the other tag's state, so we need to somehow let it know about this, if we want it to render properly. I understand that currently riot is trying to do that for me, but because it doesn't have all the information required to know whether the parent state was changed or not, you decided to do this implicit update() on every event. This is of course a possible solution, it was just not that obvious to me.

Anyway, thanks for your time and answers.

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 7, 2015

A small update for anyone fighting the same issue of unwanted parent update in the looped item, for now I ended up using the following odd-looking code in the event handler

function handler(e) {
  // ... some event handling code here
  e.preventUpdate = true; // stop parent from being updated
  this.update(); // but update the tag itself
}

yuretz commented Nov 7, 2015

A small update for anyone fighting the same issue of unwanted parent update in the looped item, for now I ended up using the following odd-looking code in the event handler

function handler(e) {
  // ... some event handling code here
  e.preventUpdate = true; // stop parent from being updated
  this.update(); // but update the tag itself
}
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 7, 2015

Member

...me that riot is gonna do this even if the element opts are the same.

no they are not In javascript two different objects are not equal (even if they contain the same key values) and for riot on any update you are just replacing the items in your collection

Basically you are saying that for the custom children tags the update event should not propagate to the parent tag. I will check whether this feature could be implemented. I would like to have other opinions about this topic

Member

GianlucaGuarini commented Nov 7, 2015

...me that riot is gonna do this even if the element opts are the same.

no they are not In javascript two different objects are not equal (even if they contain the same key values) and for riot on any update you are just replacing the items in your collection

Basically you are saying that for the custom children tags the update event should not propagate to the parent tag. I will check whether this feature could be implemented. I would like to have other opinions about this topic

@GianlucaGuarini GianlucaGuarini changed the title from Looped elements cannot receive focus to Loops - avoid to trigger "update" on the parent if the events are set on the child tag Nov 7, 2015

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 7, 2015

@GianlucaGuarini I know they are not equal in Javascript. That's why I used the words "the same" in what I wrote. And that's also why I left items assignment in the on('update') handler. I understand pretty well what's going on, I just wanted to find out the reasoning behind. Because it seems like in other cases riot is using JSON.stringify() to test object equality and not comparing them by reference.

yuretz commented Nov 7, 2015

@GianlucaGuarini I know they are not equal in Javascript. That's why I used the words "the same" in what I wrote. And that's also why I left items assignment in the on('update') handler. I understand pretty well what's going on, I just wanted to find out the reasoning behind. Because it seems like in other cases riot is using JSON.stringify() to test object equality and not comparing them by reference.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 7, 2015

Member

JSON.stringify() is really slow we use it only in case you want to loop an object and it gets called once, if we call it for any item in a collection our loops will be superslow for no reason

Member

GianlucaGuarini commented Nov 7, 2015

JSON.stringify() is really slow we use it only in case you want to loop an object and it gets called once, if we call it for any item in a collection our loops will be superslow for no reason

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 7, 2015

OK, regarding JSON.stringify() I probably misinterpreted what was written in the docs. But are all state object equality comparisons done by reference in riot?

yuretz commented Nov 7, 2015

OK, regarding JSON.stringify() I probably misinterpreted what was written in the docs. But are all state object equality comparisons done by reference in riot?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini
Member

GianlucaGuarini commented Nov 7, 2015

yes

@yuretz

This comment has been minimized.

Show comment
Hide comment
@yuretz

yuretz Nov 8, 2015

OK, thanks. This is important thing to know.

On 00:57, Sun, Nov 8, 2015 Gianluca Guarini notifications@github.com
wrote:

yes


Reply to this email directly or view it on GitHub
#1319 (comment).

yuretz commented Nov 8, 2015

OK, thanks. This is important thing to know.

On 00:57, Sun, Nov 8, 2015 Gianluca Guarini notifications@github.com
wrote:

yes


Reply to this email directly or view it on GitHub
#1319 (comment).

@GianlucaGuarini GianlucaGuarini referenced this issue Nov 8, 2015

Closed

Riot next - roadmap to riot 2.4.0 #1322

1 of 9 tasks complete
@chmanie

This comment has been minimized.

Show comment
Hide comment
@chmanie

chmanie Dec 4, 2015

Actually I found this issue having the exact same problem. It took me a long time to debug my code to get what's going on here.
The thing is, it is all about expectations. I had this mental model, that on a parent's update the children are also updated. That makes sense and is easy to understand.
Then, when I update a single child tag in a parent tag, the parent doesn't get updated (which makes sense in this mental model).
And in this case, when I have a list of child elements in the parent tag, the parent gets updated which feels very weird and didn't make any sense in my particular use-case.

chmanie commented Dec 4, 2015

Actually I found this issue having the exact same problem. It took me a long time to debug my code to get what's going on here.
The thing is, it is all about expectations. I had this mental model, that on a parent's update the children are also updated. That makes sense and is easy to understand.
Then, when I update a single child tag in a parent tag, the parent doesn't get updated (which makes sense in this mental model).
And in this case, when I have a list of child elements in the parent tag, the parent gets updated which feels very weird and didn't make any sense in my particular use-case.

@k1mbl3

This comment has been minimized.

Show comment
Hide comment
@k1mbl3

k1mbl3 Mar 14, 2016

Basically you are saying that for the custom children tags the update event should not propagate to the parent tag. I will check whether this feature could be implemented. I would like to have other opinions about this topic

It's the reactjs' virtual DOM that permits the isolation of events?

k1mbl3 commented Mar 14, 2016

Basically you are saying that for the custom children tags the update event should not propagate to the parent tag. I will check whether this feature could be implemented. I would like to have other opinions about this topic

It's the reactjs' virtual DOM that permits the isolation of events?

@GianlucaGuarini GianlucaGuarini referenced this issue Mar 19, 2016

Closed

Riot 3.0.0 roadmap #1694

14 of 16 tasks complete
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 17, 2016

Member

This patch could be problematic check this example http://plnkr.co/edit/J304qrhVtg99qcVkkaMd?p=preview

Should we disable the parent update only on the loops of custom tags?
What do you think guys?
cc @tipiirai @rsbondi @cognitom @aMarCruz @rogueg

Member

GianlucaGuarini commented Apr 17, 2016

This patch could be problematic check this example http://plnkr.co/edit/J304qrhVtg99qcVkkaMd?p=preview

Should we disable the parent update only on the loops of custom tags?
What do you think guys?
cc @tipiirai @rsbondi @cognitom @aMarCruz @rogueg

@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 18, 2016

Member

I agree with how @chmanie summed it up. I believe this line is the issue:

el = item ? getImmediateCustomParentTag(ptag) : tag

It basically says if we're talking about a loop item, go find the parent and call update on it. This is correct in the case of an anonymous tag, like:

<parent>
  <div each={item in list} onclick={foo} />
</parent>

Riot creates an anonymous tag for the div, and so it makes sense that after calling foo, we'd do an update on parent.

In @yuretz example though, edit is it's own tag, and event handlers there should have no effect on a parent tag unless the user explicitly calls it (whether it's in a list or not).

I tracked the change back to 5b99378, but neither the commit message or issue make it particularly clear if that was intended. I'd hazard a guess that if you dropped the list-specific code and just had
getImmediateCustomParent(tag).update() then it'd do the right thing.

Member

rogueg commented Apr 18, 2016

I agree with how @chmanie summed it up. I believe this line is the issue:

el = item ? getImmediateCustomParentTag(ptag) : tag

It basically says if we're talking about a loop item, go find the parent and call update on it. This is correct in the case of an anonymous tag, like:

<parent>
  <div each={item in list} onclick={foo} />
</parent>

Riot creates an anonymous tag for the div, and so it makes sense that after calling foo, we'd do an update on parent.

In @yuretz example though, edit is it's own tag, and event handlers there should have no effect on a parent tag unless the user explicitly calls it (whether it's in a list or not).

I tracked the change back to 5b99378, but neither the commit message or issue make it particularly clear if that was intended. I'd hazard a guess that if you dropped the list-specific code and just had
getImmediateCustomParent(tag).update() then it'd do the right thing.

rogueg added a commit that referenced this issue Apr 27, 2016

Implements #1319
When an event triggers, we want to traverse the DOM upwards until we find a riot tag, then call update on it.

I think that's the intent behind getImmediateCustomParent, but it didn't skip anonymous tags used by `each`. I fixed that behavior, which simplified the event handler code.

This is a breaking change, and there was one test depending on the old behavior that I had to change.
@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 27, 2016

Member

This is fixed in f3b7a0a

Because it's technically a breaking change, it'll be in riot 3.0

Member

rogueg commented Apr 27, 2016

This is fixed in f3b7a0a

Because it's technically a breaking change, it'll be in riot 3.0

@GianlucaGuarini GianlucaGuarini added this to the 3.0.0 milestone Apr 28, 2016

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 28, 2016

Member

@rogueg thanks, I think your patch can be used also to fix #1697

Member

GianlucaGuarini commented Apr 28, 2016

@rogueg thanks, I think your patch can be used also to fix #1697

@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 28, 2016

Member

@GianlucaGuarini I don't think it does, but I'm happy to take a look at that issue if it's helpful.

Member

rogueg commented Apr 28, 2016

@GianlucaGuarini I don't think it does, but I'm happy to take a look at that issue if it's helpful.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 28, 2016

Member

@rogueg I mean the patch is really simple, we need to change just this line :

 // inherit properties from the parent
if (!self._hasImpl) inheritFromParent()
Member

GianlucaGuarini commented Apr 28, 2016

@rogueg I mean the patch is really simple, we need to change just this line :

 // inherit properties from the parent
if (!self._hasImpl) inheritFromParent()
@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 28, 2016

Member

Agreed! I didn't actually add hasImpl, I just exposed it.

Member

rogueg commented Apr 28, 2016

Agreed! I didn't actually add hasImpl, I just exposed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment