Skip to content
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

When updating a tag with a loop, rendered result doesn't always reflect the updated data #990

Closed
cdanielw opened this Issue Jul 16, 2015 · 153 comments

Comments

Projects
None yet
@cdanielw
Copy link

commented Jul 16, 2015

The below fiddle recreates the issue. Checking an item should remove it. However, checking any but the last item with riot 2.2.2, while the item is removed, the item below becomes checked. It works fine with 2.1.0 and 2.0.15.

http://jsfiddle.net/cdanielw/bkw5ommu/4/

@danieltian

This comment has been minimized.

Copy link

commented Jul 16, 2015

The source of this problem is that the checkbox event is still performing its default behavior, leading to the issue you've observed. The fix is to simply add an e.preventDefault() to your toggled(e) function:

https://jsfiddle.net/bkw5ommu/7/

Riot tags will automatically use e.preventDefault() for stuff like links, but it looks like checkboxes isn't one of them (@GianlucaGuarini, bug?).

Edit: not a bug according to the docs linked by @rsbondi.

@r4j4h

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2015

Riot tags will automatically use e.preventDefault() for stuff like links

Is this documented somewhere?

@rsbondi

This comment has been minimized.

@weepy

This comment has been minimized.

Copy link

commented Jul 16, 2015

What is the reasoning behind why checkboxes aren't automatically handled ?

@danieltian

This comment has been minimized.

Copy link

commented Jul 16, 2015

@weepy Probably because checking a checkbox/clicking a radio button and having it selected is the behavior you'd want in the majority of cases. With the e.preventDefault(), the checkbox/radio won't get selected, and you'll have to do it manually.

In the case of this issue, I'm just taking a stab at the dark, but what's probably happening is that the toggled() function is running, followed by the this.on('update') listener. Only after that does the default event happen, but the checkbox that it's supposed to be checking has been removed from the DOM, so the event just finds the next child and checks that instead (if it exists).

@cdanielw

This comment has been minimized.

Copy link
Author

commented Jul 17, 2015

My take on this is that it’s due to the way leftover items are unmounted in each.js – take this with a grain of salt though, since I’m new to Riot.

  var frag = document.createDocumentFragment(),
      i = tags.length,
      j = items.length

  // unmount leftover items
  while (i > j) {
    tags[--i].unmount()
    tags.splice(i, 1)
  }

When an item in the middle is removed, the last tag will be unmounted, not the tag previously bound to the removed item. Then the remaining items are rebound to the tags. This doesn’t take into account that the DOM nodes themselves are stateful. The below fiddle shows this more clearly. Here, the background color is set on the node to be removed.

https://jsfiddle.net/cdanielw/bkw5ommu/8/

@rsbondi rsbondi added the bug label Jul 18, 2015

@rsbondi

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2015

Calling e.preventDefault is only masking the issue as pointed out in the example by @cdanielw

@oderwat

This comment has been minimized.

Copy link

commented Jul 21, 2015

We lost a lot of time because of this behavior and had to rewrite a lot of code without fully realizing that this confirms as bug.

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

We discussed a lot about this here:
#484

@oderwat

This comment has been minimized.

Copy link

commented Jul 29, 2015

I feared that #484 will be brought up. But that talked imho about the order of the dom nodes. Now we also talk about all their content and total loss of sync between the state in riot and the dom.

That optimization destroyed multiple days of menpower for us as we tried to recover from switching to 2.2 by changing our code. I should have simply not ordered the team to adapt to 2.2. "in any case" and stayed with 2.1 or 2.0.15 which worked much more reliable in that regard.

Now there is no end to it and things are popping up here an there. I think the behavior from the jsfiddle @cdanielw contributed is enough to qualify this as a problem which needs to get fixed. You can't reset all properties in every update all of the time manually just because an entry got deleted.

BTW: Having an optimization which requests to manually bring dom nodes into sync again and again is not worth it. It is always measurably faster to dump core functionality and let it to the implementor to fix that. So either add another each which guarantees sync or fix the current one such that it works as people expect it to work.

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2015

I see this as the same problem when you lose the reference to the DOM elements.

Only way to fix the issue for now is to move all the logic to the template, since you can't trust the JS (DOM and data might lose their relationship in any point).

And again the only way to efficiently keep the DOM references is to introduce a unique id for looping items. But I'm repeating myself here.

@GianlucaGuarini GianlucaGuarini self-assigned this Aug 1, 2015

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

@oderwat

That optimization destroyed multiple days of menpower for us as we tried to recover from switching to 2.2 by changing our code. I should have simply not ordered the team to adapt to 2.2. "in any case" and stayed with 2.1 or 2.0.15 which worked much more reliable in that regard.

I am sorry that our optimization did not fit with your project legacy code, could you show us a simple example where this new riot behavior breaks your application I am curious to see what's the problem

Now there is no end to it and things are popping up here an there. I think the behavior from the jsfiddle @cdanielw contributed is enough to qualify this as a problem which needs to get fixed. You can't reset all properties in every update all of the time manually just because an entry got deleted.

from riot 2.0.0 to riot 2.1 anytime a child node was removed all the other expressions where evaluated anyway, so in this case nothing is changed. What do you mean by resetting all the properties just because an entry got deleted? Your components should be in always in sync with the items collection, and you should let riot updating your DOM nodes, without doing it manually

Thank you guys for raising this up, I would like to have more opinions and examples about this riot behavior

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

@cdanielw riot updates just the expressions in a loop and it does not keep track of the DOM properties, your example could be rewritten in this way (no need of e.preventDefault)
This optimization makes riot one of the fastest framework and maybe it should be better documented, but I am not sure we need to switch back to the older riot looping strategy
check yourself!
current riot in the dev branch
riot 2.1.0

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

If you have the following data (in the DOM):

[{id: 1, title: 'Item 1'}, {id: 2, title: 'Item 2'}, {id: 3, title: 'Item 3'}]

..and you remove the first item:

[{id: 2, title: 'Item 2'}, {id: 3, title: 'Item 3'}]

What you'd expect to happen:

• 1 gets removed

What really happens:

• 1 becomes 2 (id: 1 -> id: 2, title: 'Item 1' > title: 'Item 2')
• 2 becomes 3 (id: 2 -> id: 3, title: 'Item 2' > title: 'Item 3')
• 3 gets removed

I think this is the case :)

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

Yes I see, this could be easily solved getting the index of the items to remove, but how to handle a data set like this:

var items = ['foo', 'bar', 4, 'foo', 3, 4, 3, 'bar']
// update
items = ['foo', 'bar', 4, 3, 3]
// which tag must be removed here? indexOf will get always the first match

The current sync method seems to me the easiest and the most consistent

@oderwat

This comment has been minimized.

Copy link

commented Aug 3, 2015

As @pakastin says... If you set a style or form input value to any of those and remove element 1. They will suddenly appear as part of entry two.

@GianlucaGuarini I really don't feel like "inventing" another example. Those listed here are all showing the problem. The example you rewrote totally skips modifying the background color of the dom node. But that is what happens to be the problem (see the modified version of your code: http://jsfiddle.net/o9xzg0jy/1/)

In our code we insert new elements in the middle of the array which gets rendered with each as custom tags. What happens is that everything which is nor explicitly "overwritten" inside the update event, will stay well unmodified instead that the dom nodes get shifted/updated.

It is also pretty lame to have custom tags with their own data (opts or inside this) but need to access the entries from parent using an index all the time. That was not needed in Riot 2.0 afaik.

I don't say that it is not cool to have faster updates where it is working but I think there should be an opt in to slower updates but guaranteed sync with the dom nodes.

Can't we have both?

@oderwat

This comment has been minimized.

Copy link

commented Aug 3, 2015

@GianlucaGuarini about your example... the items needed to be removed are the ones which "object address" are not contained in the update anymore. We use push() and splice() on the items. Data is objects. Used with each and custom tags the data corresponds to the custom tag itself. Maybe we do it all wrong :) But that worked wonderful in 2.0 ...

@weepy

This comment has been minimized.

Copy link

commented Aug 3, 2015

How often in real code do you need to render something like ['foo', 'bar', 4, 'foo', 3, 4, 3, 'bar'] vs an array of objects?
Not very often methinks?

On Mon, 3 Aug 2015 22:18 Hans Raaf notifications@github.com wrote:

As @pakastin https://github.com/pakastin says... If you set a style or
form input value to any of those and remove element 1. They will suddenly
appear as part of entry two.

@GianlucaGuarini https://github.com/GianlucaGuarini I really don't feel
like "inventing" another example. Those listed here are all showing the
problem. The example you rewrote totally skips modifying the background
color of the dom node. But that is what happens to be the problem (see the
modified version of your code: http://jsfiddle.net/o9xzg0jy/1/)

In our code we insert new elements in the middle of the array which gets
rendered with each as custom tags. What happens is that everything which is
nor explicitly "overwritten" inside the update event, will stay well
unmodified instead that the dom nodes get shifted/updated.

It is also pretty lame to have custom tags with their own data (opts or
inside this) but need to access the entries from parent using an index all
the time. That was not needed in Riot 2.0 afaik.

I don't say that it is not cool to have faster updates where it is working
but I think there should be an opt in to slower updates but guaranteed sync
with the dom nodes.

Can't we have both?


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

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

@GianlucaGuarini I really don't feel like "inventing" another example. Those listed here are all showing the problem. The example you rewrote totally skips modifying the background color of the dom node. But that is what happens to be the problem (see the modified version of your code: http://jsfiddle.net/o9xzg0jy/1/)

Riot has a fast and reliable template engine, why should you update the DOM without using it http://jsfiddle.net/o9xzg0jy/2/ ?

In our code we insert new elements in the middle of the array which gets rendered with each as custom tags. What happens is that everything which is nor explicitly "overwritten" inside the update event, will stay well unmodified instead that the dom nodes get shifted/updated.

Probably in your app you are mixing riot expressions, with custom jquery plugins, and other manual DOM updates, in this case you are right the current riot loops will fail I must admit that this can be considered a problem

It is also pretty lame to have custom tags with their own data (opts or inside this) but need to access the entries from parent using an index all the time. That was not needed in Riot 2.0 afaik.

This was just an example in real world application you need a flux architecture

I am still curious to see a real world example of your app, where you have problems updating your DOM. I would be really glad to help you if I could

@oderwat

This comment has been minimized.

Copy link

commented Aug 3, 2015

@GianlucaGuarini I could give you access to our internal git repository with the project. It would be much appreciated if you could give us some pointers - even if we did something pretty wrong. Actually it is not me who is coding it and I have limited knowledge about the problems in detail. I just try to supervise the work without using riot myself very much.

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

I don't know if I have time to check your entire project, but I guess probably you are doing something wrong. Just let some of your devs posting a jsfiddle where the current riot loops fail, so I will understand where is the issue. In general I strongly recommend a deep tour of our useful links in our readme file before starting any complex project
Please check also RiotControl to understand how could be simple to manage the riot loops

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2015

Another real world example:

var element = {id: 1, title: 'Element 1'}
var element2 = {id: 2, title: 'Element 2'}
var element3 = {id: 3, title: 'Element 3'}

var item = {id: 1, elements: []}
var item2 = {id: 2, elements: [element, element2, element3]}
var item3 = {id: 3, elements: []}

The change:

[item, item2, item3]

->

[item2, item3]

What you'd expect:
• Item 1 gets removed

What really happens:
• Item 1 becomes item 2 (id: 1 -> id: 2 + element, element2 and element3 gets created)
• Item 2 becomes item 3 (id: 2 -> id: 3 + element, element2 and element3 gets removed)
• Item 3 gets removed

Think about a situation where you have lots of nested elements getting created and removed just because one item got removed (or reordered)..

@oderwat

This comment has been minimized.

Copy link

commented Aug 3, 2015

We know what is wrong @GianlucaGuarini ... you don't care about sync between the dom and the data anymore since 2.1 ... thats the problem. Choosing the best workaround is the problem here! It worked flawlessly in 2.0 there is no doubt about that.

You already have a fiddle where it fails ... if you say: Well we need to update every aspect of every dom node manually, yeah.. thats what I said before: The optimization kills the old code and one has to do it all manually. If I set an background color on mount I don't want to set it on every update again. I just want to set what changes. And again. This seemed to work in 2.0 and is now broken. As is re-ordering the elements. Which was a problem we had before and solved it by rewriting the nice and clean code into a mess of manually updates in the update event.

@oderwat

This comment has been minimized.

Copy link

commented Aug 3, 2015

@GianlucaGuarini BTW: First saying your would like to see the real use case (code) and then say you don't have the time is not helping much. Either you want to see the real code or you not :) And, yes we use RiotControl already :)... We also had to remove nearly all nested tags to get stuff back to a working state as everything seems to break with 2.2 and nested tags :(

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Aug 3, 2015

@oderwat We will release riot 2.2.3 really soon. I will come back to this issue with riot 2.3 I swear 😄
Hoping to find a solution that makes anyone happy

GianlucaGuarini added a commit that referenced this issue Oct 5, 2015

GianlucaGuarini added a commit that referenced this issue Oct 5, 2015

GianlucaGuarini added a commit that referenced this issue Oct 5, 2015

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

this issue now is fixed and the patch is available in the dev branch. To enable the dom reordering you should set reorder=true on dom node you want to loop. The issues with unshift and the wrong children removal were solved also without enabling the reorder attribute. I have added some tests to avoid this issue in future. The riot loops became a bit slower but more reliable.

@tipiirai

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2015

Holy crap.

I haven't tried this yet, but looks like you did a fantastic job. This was certainly a pain in the ass. Thank you for the hard push!

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Oct 6, 2015

@tipiirai yeah it was but without @rsbondi I could not fix this issue. Thank you guys for your help

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

@GianlucaGuarini: still saying DOM reorder is slow?
https://twitter.com/pakastin/status/651581181910208512

Thanks for challenging me! ;)

@tipiirai

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

Awesome performance there on frzr. Congrats!

We obviously hope to get to the same speed level, but it's much harder when you need to support unlimited nesting of custom tags, which can also have nested loops and conditionals.

This is an ongoing process. Maybe profiling tools can help.

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

I know, and still I'm not comparing - just trying to push the browser limits and show what could be done. And FRZR wouldn't exist without Riot: I didn't understand anything about this stuff before I fell in love with the simplicity of Riot.

Profiling helped a lot. I will try profile Riot also when I figure out how to use this new reorder parameter.

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

I close this bug it got fixed we can move forward

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

so definitely frzr.js is better than riot maybe I should start using it! 😹

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

I sense some sarcasm there.. :) Well, FRZR is mainly for my personal use, but I just wanted to share the research. Of course Riot is much easier to use i.e. because of templating, that's something I intentionally left out. FRZR is more of a javascript-first view engine.

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

but I just want to share the research

I use to call it spam

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

Hey, you're now using the new efficient reorder method in Riot as well, which came out as a side product from my FRZR tests. Show some respect. Also the only reason I shared the tests here was you not believing me that reordering is not slow (which I'm grateful to you, because it pushed me towards researching even better performance).

Sometimes it's easier to iterate (& benchmark) ideas with a fresh project.

FRZR is not competing with Riot, you should already know that.

@pakastin

This comment has been minimized.

@oderwat

This comment has been minimized.

Copy link

commented Oct 7, 2015

@GianlucaGuarini Why don't you realize that: #484 (comment) was simply a wrong decision. That broke projects and in the end cost people a lot of time and money. @pakastin already took part of the conversation at that time and even if you would not use any of his ideas hey saw the weight of the problem you didn't. You even wrote "Riot 2.2.0 is finally out! I will close this issue hoping our users will not complain the old each method 😄" :(

@oderwat

This comment has been minimized.

Copy link

commented Oct 7, 2015

To make it clear: I think that Riot with the new optional optimized "reordering" is much cooler that "being as fast as anyone else without reordering". And that was (imho) the spirit @pakastin wanted to bring to the project. Thanks @ALL btw...

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

I even tried to say back then many times that dropping reordering will cause problems, i.e.:
#484 (comment)

Btw. this fix doesn't seem to reorder DOM elements anymore, am I right?
That's a breaking change and might cause problems for some. At least for me reordering is a required feature.

and:
#484 (comment)

Think that you have a tree structure, where you render every depth concurrently. If you move a node having a deep structure, then it's easier to render only the DOM reordering, and not removing components in all depths and recreating them in the new place.. (If replacing branch doesn't have equal deep structure). There are lots of other use cases where reordering is needed.

Remember that the loops in Riot are not only data loops, but also there could be components involved.

@oderwat

This comment has been minimized.

Copy link

commented Oct 7, 2015

Let's not beat the dead horse :)

I take the "I close this bug it got fixed we can move forward" as the essence. After all I am happy that we can use such a mature project basically for free!

@pakastin

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

I agree :)

@rsbondi

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

I tend to think that the default should be the reorder mode, that works in all cases, but is slower, and if you don't need reordering but need speed, then you should set an attribute no-reorder or something. Also, I would be happy not having any attribute and living with the slower speed, which is not an issue unless there is an unreasonable amount of tags mounted(I am a pagination fan but that does not seem to be the trend these days), that would keep riot more simple, one less parameter to learn. Only my opinion, would like to hear others.

@oderwat

This comment has been minimized.

Copy link

commented Oct 7, 2015

I go with @rsbondi but would like an opt-in to an optional faster version (noreoder). It could also be each and fasteach or noreach or raweach instead of an option. Which would make it possible to track usage more easily.

@GianlucaGuarini

This comment has been minimized.

Copy link
Member

commented Oct 7, 2015

@rsbondi @oderwat could you please open a new issue/discussion about the reorder attribute?

@rsbondi rsbondi referenced this issue Oct 7, 2015

Closed

Reordering #1258

@Mouvedia

This comment has been minimized.

Copy link

commented Oct 7, 2015

👍 @rsbondi

@iamliqiang

This comment has been minimized.

Copy link

commented Oct 13, 2015

congratulation, 990 was fixed ! thanks for riot.js team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.