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

Virtual elements disappear after tag update #1659

Closed
believer-ufa opened this Issue Mar 7, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@believer-ufa

believer-ufa commented Mar 7, 2016

Example on plunker

Windows 7, Chrome 49
Riot.js 2.3.16

  • Bug

@rsbondi rsbondi self-assigned this Mar 7, 2016

@rsbondi rsbondi removed the to verify label Mar 12, 2016

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Mar 12, 2016

Member

There are several things wrong here, currently if and each do not work on virtual, this is fixed in the next branch, here is a work around for now. The other issue is #1519 which is marked as fixed, not sure of the current status, it is marked as 3.0 milestone so prabably in next branch? It still appears to be an issue in the current release.

Member

rsbondi commented Mar 12, 2016

There are several things wrong here, currently if and each do not work on virtual, this is fixed in the next branch, here is a work around for now. The other issue is #1519 which is marked as fixed, not sure of the current status, it is marked as 3.0 milestone so prabably in next branch? It still appears to be an issue in the current release.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Mar 12, 2016

Member

@rsbondi do you still see the #1519 issue in the next branch?

Member

GianlucaGuarini commented Mar 12, 2016

@rsbondi do you still see the #1519 issue in the next branch?

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Mar 12, 2016

Member

With nested In this example the issue exists, if you comment out the "Real" section the example works, or if you move the "nested" section before the real.it works also.

if (child) parent.tags[tagName] = tags at the end of _each seems to not recognize the component2 as a tag and removes them. The example works with this line removed, but certainly not a reasonable fix.

Also a side note, it seems the inheritance has changed in next, I had to chage peron.name/age to opts.name/age in component2

Member

rsbondi commented Mar 12, 2016

With nested In this example the issue exists, if you comment out the "Real" section the example works, or if you move the "nested" section before the real.it works also.

if (child) parent.tags[tagName] = tags at the end of _each seems to not recognize the component2 as a tag and removes them. The example works with this line removed, but certainly not a reasonable fix.

Also a side note, it seems the inheritance has changed in next, I had to chage peron.name/age to opts.name/age in component2

@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Mar 15, 2016

Member

This should be fixed in next by #1658. It's probably worth making sure we have test coverage of if each and virtual all working together.

Member

rogueg commented Mar 15, 2016

This should be fixed in next by #1658. It's probably worth making sure we have test coverage of if each and virtual all working together.

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Apr 17, 2016

Member

@rogueg I tried to verify this issue with the latest next build. Take a look here and look at the explanation in the tag comments

Member

rsbondi commented Apr 17, 2016

@rogueg I tried to verify this issue with the latest next build. Take a look here and look at the explanation in the tag comments

@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 27, 2016

Member

I've got a fix for this. There was a bug with how riot handled each and if together on a custom tag.

@believer-ufa there were also some bugs in your original example:

  • <virtual name={person} each={person in people} - here, person is actually the DOM node, rather than a js object, because of http://riotjs.com/guide/#named-elements
  • In component2 you should use opts.name instead of person.name. It only worked in the "real" case because of some inheritance weirdness that we'd like to fix in riot 3.

Putting it all together, try out this plunk and tell me if it behaves as expected.
http://plnkr.co/edit/MBTfCCgo129dICKTXiTA?p=preview

Member

rogueg commented Apr 27, 2016

I've got a fix for this. There was a bug with how riot handled each and if together on a custom tag.

@believer-ufa there were also some bugs in your original example:

  • <virtual name={person} each={person in people} - here, person is actually the DOM node, rather than a js object, because of http://riotjs.com/guide/#named-elements
  • In component2 you should use opts.name instead of person.name. It only worked in the "real" case because of some inheritance weirdness that we'd like to fix in riot 3.

Putting it all together, try out this plunk and tell me if it behaves as expected.
http://plnkr.co/edit/MBTfCCgo129dICKTXiTA?p=preview

@rsbondi

This comment has been minimized.

Show comment
Hide comment
@rsbondi

rsbondi Apr 27, 2016

Member

That looks like the expected behavior

Member

rsbondi commented Apr 27, 2016

That looks like the expected behavior

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 27, 2016

Member

@rogueg will your patch be merged in riot@3.0.0?

Member

GianlucaGuarini commented Apr 27, 2016

@rogueg will your patch be merged in riot@3.0.0?

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

Have `each` skip items where an `if` is false.
#1659 brought up an issue where you'd have both `each` and `if` on a custom tag. In that case, the each code would actually create an instance of the tag, even when the `if` expressions was false.

To get around this, we just have `each` also evaluate the `if` and skip items where it's false. That might sound like mixing concerns a bit too much, but I actually think the resulting code is easier to read. It's also probably faster, since we avoid creating and parsing DOM in the false case.
@rogueg

This comment has been minimized.

Show comment
Hide comment
@rogueg

rogueg Apr 27, 2016

Member

Yup. 5f1eda4 should fix this.

Member

rogueg commented Apr 27, 2016

Yup. 5f1eda4 should fix this.

@rogueg rogueg closed this Apr 27, 2016

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Apr 27, 2016

Member

I will close this issue once riot@3.0.0 will be released. thanks @rogueg

Member

GianlucaGuarini commented Apr 27, 2016

I will close this issue once riot@3.0.0 will be released. thanks @rogueg

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

@rsbondi rsbondi removed their assignment Jul 24, 2016

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