'each' loops are triggering mount events when a global mixin is used #2056

Closed
jpoppe opened this Issue Nov 3, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@jpoppe

jpoppe commented Nov 3, 2016

Help us to manage our issues by answering the following:

  1. Describe your issue:
    When a global mixin is used 'each' items are triggering mount events. (I really hope I am not doing something stupid here).

  2. Can you reproduce the issue?

Post the link using one of our bug report templates:

Console output:

from global mixin: mounted - my-tag mixins.js (line 6)
from baseMixin: mounted - my-tag mixins.js (line 16)
6 * from global mixin: mounted - li

Expected console output:

from global mixin: mounted - my-tag mixins.js (line 6)
from baseMixin: mounted - my-tag mixins.js (line 16)
  1. On which browser/OS does the issue appear?
    Tested on latest Firefox/Chrome and Chromium (using Arch Linux).

  2. Which version of Riot does it affect?
    Next and master.

  3. How would you tag this issue?

  • Question
  • Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@sourcegr

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Nov 3, 2016

I am in no way javascript experienced programmer
I am completely new to riot
never looked at the code of riot.

And I am completely new to github / not even sure what a pull reqest is
I dont know how to clone the repo or do the tests
but I think that in line 1671

      if (!isInStub(self.root)) {
        self.parent.isMounted = self.isMounted = true
        self.trigger('mount') /* this line */
      }

it should be

      if (!isInStub(self.root)) {
        self.parent.isMounted = self.isMounted = true
        !isLoop && self.trigger('mount') /* this line */
      }

could be wrong though...

feel free to delete the comment if in any way is wrong/comfusing or something not suitable.

sourcegr commented Nov 3, 2016

I am in no way javascript experienced programmer
I am completely new to riot
never looked at the code of riot.

And I am completely new to github / not even sure what a pull reqest is
I dont know how to clone the repo or do the tests
but I think that in line 1671

      if (!isInStub(self.root)) {
        self.parent.isMounted = self.isMounted = true
        self.trigger('mount') /* this line */
      }

it should be

      if (!isInStub(self.root)) {
        self.parent.isMounted = self.isMounted = true
        !isLoop && self.trigger('mount') /* this line */
      }

could be wrong though...

feel free to delete the comment if in any way is wrong/comfusing or something not suitable.

@jpoppe

This comment has been minimized.

Show comment
Hide comment
@jpoppe

jpoppe Nov 4, 2016

@papas-source thanks for your suggestion!

jpoppe commented Nov 4, 2016

@papas-source thanks for your suggestion!

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 5, 2016

Member

@jpoppe from our doc

A new context is created for each item. These are tag instances. When loops are nested, all the children tags in the loop inherit any of their parent loop’s properties and methods they themselves have undefined. In this way, Riot avoids overriding things that should not be overridden by the parent tag.

This means that for riot tags created in an each loop will use the global mixins exactly like the other tags. In riot the each directive will create a new tag instance for each element in the loop independently if it's an anonymous tag ( tags not registered via riot.tag ) like in your case, or if it's a registered one. I am not sure if we should skip the global mixins for the anonymous tags since they could be also used for useful stuff like animations http://plnkr.co/edit/DlU6ib3Pahj0p7prRRBp?p=preview

I would recommend to use the named mixins in your case

Member

GianlucaGuarini commented Nov 5, 2016

@jpoppe from our doc

A new context is created for each item. These are tag instances. When loops are nested, all the children tags in the loop inherit any of their parent loop’s properties and methods they themselves have undefined. In this way, Riot avoids overriding things that should not be overridden by the parent tag.

This means that for riot tags created in an each loop will use the global mixins exactly like the other tags. In riot the each directive will create a new tag instance for each element in the loop independently if it's an anonymous tag ( tags not registered via riot.tag ) like in your case, or if it's a registered one. I am not sure if we should skip the global mixins for the anonymous tags since they could be also used for useful stuff like animations http://plnkr.co/edit/DlU6ib3Pahj0p7prRRBp?p=preview

I would recommend to use the named mixins in your case

@sourcegr

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Nov 6, 2016

Good evening

I believe that since a lot of anomymus tags (and I mean only loops) happen in a tag, the default behavior should be for them to NOT inherit the mixin of the parent tag. Normaly, the mount event for a tag is ment to fire only once in the begining of the lifecyfle of the tag

For animations, (since I cannot find some other possible use for such a feature), it would be better, and make better sense, if it was declared in a way like (adopted your example)

<li data-is="fade-in-element" each={ items }>item</li>

Thank you - and sorry I am not a fluent english speaker :/

sourcegr commented Nov 6, 2016

Good evening

I believe that since a lot of anomymus tags (and I mean only loops) happen in a tag, the default behavior should be for them to NOT inherit the mixin of the parent tag. Normaly, the mount event for a tag is ment to fire only once in the begining of the lifecyfle of the tag

For animations, (since I cannot find some other possible use for such a feature), it would be better, and make better sense, if it was declared in a way like (adopted your example)

<li data-is="fade-in-element" each={ items }>item</li>

Thank you - and sorry I am not a fluent english speaker :/

@jpoppe

This comment has been minimized.

Show comment
Hide comment
@jpoppe

jpoppe Nov 7, 2016

@GianlucaGuarini thanks a lot for your clear explanation, I did not expect this behaviour but could also live with named mixins.

jpoppe commented Nov 7, 2016

@GianlucaGuarini thanks a lot for your clear explanation, I did not expect this behaviour but could also live with named mixins.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Feb 6, 2017

Member

fixed in riot@3.2.0

Member

GianlucaGuarini commented Feb 6, 2017

fixed in riot@3.2.0

@jpoppe

This comment has been minimized.

Show comment
Hide comment
@jpoppe

jpoppe Feb 6, 2017

@GianlucaGuarini Thanks again, you guys are awesome!!

jpoppe commented Feb 6, 2017

@GianlucaGuarini Thanks again, you guys are awesome!!

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