Riot: Life cycle issues with if statements

[luffs]

The html renders correctly to the page, but the life cycle events are not emitting correctly.

1. the <fish/> is hidden at start, so its mount event shouldn't run.
2. the mount event should be triggered when it's added.
3. the unmount event should be triggered when it's removed.

http://jsfiddle.net/60rk01ux/1/

<test>
    <div if={(current == 'fish')}>
        <fish></fish>
    </div>
    <div if={(current == 'cat')}>
        <cat></cat>
    </div>
    <script>
    this.current = 'cat';
    </script>
</test>

<cat>
    <h1>=^..^=</h1>
    <button onclick={activateFish}>Activate fish</button>
    <script>
    activateFish(){
        this.parent.update({current: 'fish'})
    }
    </script>
</cat>

<fish>
    <h1> {"<')))><"} </h1>
    <button onclick={activateCat}>Activate cat</button>
    <script>
    activateCat(){
        this.parent.update({current: 'cat'})
    }
    this.on('mount', function(){
        alert('fish mounted')
    });
    this.on('unmount', function(){
        alert('fish unmounted')
    });
    </script>
</fish>

[tipiirai]

You're right. These events should indeed be fired.

Thanks.

[cfenzo]

@tipiirai continuing from #423, just to get a clear picture of how the lifecycle of hidden/shown tags are intended :)

Using @luffs example, is this the intended behavior for a hiding/showing custom tags?

1. <fish /> is hidden by default, no part of the lifecycle is executed
2. <fish /> is shown
   
   
   
   1. The tags JavaScript logic is executed
   
   
   2. HTML expressions are calculated and “update” event is fired
   
   
   3. The tag is mounted on the page and “mount” event is fired
   
   
3. <fish /> is hidden
   
   
   
   1. The tags "unmount" event is fired
   
   
4. <fish /> is shown again
   
   
   
   1. The tags JavaScript logic is executed
   
   
   2. HTML expressions are calculated and “update” event is fired
   
   
   3. The tag is mounted on the page and “mount” event is fired
   
   

Or is the the intended behavior that the logic is executed (and the expressions calculated) even when the tag is hidden, and that only the events are triggered when hiding/showing?

[cuibonobo]

I'm also getting bitten by if life cycle issues, and I boiled it down to a much simpler example:

<test>
    <div if={ false }>
        { alertOnExecute() }
    </div>
    <script>
    alertOnExecute(){
        alert('The code was executed!');
    }
    </script>
</test>

http://jsfiddle.net/jov9zyeL/

Despite the fact that the if statement is false, alertOnExecute() always runs. I can work around it, but it _feels_ wrong.

[GianlucaGuarini]

@cfenzo with the latest riot release the fish tag gets mounted only once and the unmount event never gets fired. I think we could close this but

[AndreasHeintze]

@jenmontes I have just made a solution for your problem. When an if-expression is present and it resolves to false, the dom node is removed, but any nested expressions are still being updated.

Will make a PR as soon as I can.

Do you want me to find a solution to the mount/unmount problem above too? It's a completely different problem than the problem @jenmontes wrote about above which I have a working solution for. I can take a look at it if you want me to.

[Avnerus]

+1 for finding a solution to the mount/unmount on if conditions. For example, we have JQuery code that should run only once and only when the elements are actually displayed. The most intuitive way would be to run it on mount, but with the current flow, it runs even when the elements are not displayed which causes issues.

[AndreasHeintze]

I'm still working on it although I have had a little less time over to fix it. I had to turn around the mounting to make it work. That is instead of as it is now, where the children is mounted before the parent by the toggle function I have turned it around and updating the expressions on the parent first, then the children. It's because I need to know if the if-expressions on the parent turned out to be true or false. If false, any child tag under that if-expression should not be updated. I also have to look closer on the each-childs and see if that works or if I have to do something about them too. ..and the mounting order test fails, so I need to change the test too. ..but there was a problem with the opts not being there on one tag, so I have a little more to fix before I can update the PR.

[Avnerus]

I encountered a weird problem with the opts not being transferred well to a tag that follows an each loop. A workaround was to wrap the loop in a <div>. I will try to recreate that and open an issue.
About the mount order, maybe it's possible to just propagate the parent's if expression's result to the children so they can test it before mounting?

[AndreasHeintze]

About the mount order, maybe it's possible to just propagate the parent's if expression's result to the children so they can test it before mounting?

That is what I'm doing. I write a dom reference to each tag that points to it's closest dom-parent that has an if-expression and I do set a var (removed) on that dom-element if the result of the if-expression is false. Then in update function I do a check of to see if removed is set on that element, if it is, I just do a return.

I need to fix the each loop children. They are being mounted before their parent.

[Avnerus]

So actually why isn't is possible to reverse the order and have each parent control the construction/destruction of its children instead of keeping a reference?

[Avnerus]

@GianlucaGuarini Thought I would answer here to what you wrote on https://github.com/muut/riotjs/issues/773.
The reason I considered it a bug is because I understood from the documentation that when a tag's mount event is fired, you can rely on the tag's html being in the page so you can run JQuery etc. But that is not the case with if statements. mount is triggered but the html is not present. Maybe it can be clarified in the documentation? I think many people might run into this confusion.
I tried to follow up on the cat/fish example with the JSFiddle bug template:
https://jsfiddle.net/4x5t1sLw/2/

[AndreasHeintze]

I agree with @Avnerus here, that it feels more logical if the mount event are fired when the tag is actually mounted in the dom and the unmount event should be fired when it's removed from the dom.

[cfenzo]

I agree with the points made by @luffs , @jenmontes , @Avnerus and @AndreasHeintze here..

Usually, an if-block dictates _"if true: do this, if false: do not execute/touch"_.
(And in other frameworks this includes adding/removing (mounting/unmounting and initializing) custom-tags inside the block).
Experiencing this to not be the case with riot was why I asked for a clarification on how the lifecycle behavior where intended earlier.

With the current behavior, the if-blocks is in reality a cosmetic variation of hide/show (hiding html with js instead of css), and not usable for preventing code from running until a condition is met.

This is how I've had to solve _"proper"_ if now, using each (loosing _"this"_ context as a consequence ):

<foo each={ _if(switch) }></foo>

_if(expr){
    return expr?[expr]:[];
}

Example: http://jsfiddle.net/qxe4s6cw/5/

I hope we can get a clear picture of how if-blocks should behave in riot, and if that turns out to be the current behavior it should be clearly stated in the docs with a clear warning.

Also, if hide/show is the if behavior wanted in riot, we need more lifecycle events on tags to know when the html is added/removed from the document (visible/invisible or hide/show).

[pakastin]

I think events should be something like this:

init : new item
mount : added to DOM
update : render update
updated : render updated
unmount : removed from DOM
destroy : removed item

[geordee]

mounted and unmounted?

[pakastin]

Yes, actually:
mount : mounting to DOM
mounted : mounted to DOM
unmount : unmounting from DOM
unmounted : unmounted from DOM

Some things need to do before mounting and some things after, so would make sense..

[Avnerus]

@AndreasHeintze Were you able to get it working eventually? Any way I can help?
Maybe it's best to first just fix the mount event, so to not break the API.

[Avnerus]

OK so I tried to create a kind of compromising solution in my fork:
https://github.com/ListnPlay/riotjs/tree/dev
Basically instead of changing the way mount/unmount works, I just delay the mount event until the tag is actually in the dom, by checking up the dom tree for aninStub flag that is set if that resolves to false. I also added a test called <if-mount> to test it.
What do you think about this direction?
I think for me it's good enough because it allows me to do the JQuery stuff on mount.
The question remains about the unmount event. Since the the html is not actually deleted, but only moves to a stub, maybe it's best not to trigger unmount and mount every time the condition changes, and only mount once.

** Update: Added more tests and made the inStub utility to check up the tree.

[GianlucaGuarini]

thanks to @Avnerus we could close this issue

[GianlucaGuarini]

The issue is not 100% fixed, we will keep this issue opened until we will find a better solution

[avimar]

So you fixed that the mount event is fired only once it's visible, but it still otherwise executes the code?
This _works_ but it's still a bit cumbersome and unexpected...

[gcr]

Typically, it's good practice to put special actions into your on("mount", ...) handler. I like the current behavior; I think it makes sense for initialization code to be evaluated before the object is technically mounted.

<tag-depending-on-ajax-items>
    <li each="{result in this.results}">{result}</li>

    // Initialization (evaluated before mounting)
    this.results = [];

    // Mounting
    this.on("mount", function(){
        var self = this;
        fetch(..., function(data){ self.results = data; self.update(); });
    });
</tag-depending-on-ajax-items>

[avimar]

@gcr but all that code currently runs even if it won't mount at all.
From both a performance and a normative coding expectation, this is a bug, not a feature...

[rsbondi]

@avimar this has been fixed and is in the @next branch

npm install riot@next

[markwylde]

I'm not sure this one is fixed, but maybe I'm missing something. The mount is definitely working, but the unmount event is still never called.

See the updated version on your plunker code:
http://plnkr.co/edit/3jAwBdfugl02NfKrozJ6?p=preview

I'm a doing something silly, or should the unmount be fired when the conditional is set to false?

[GianlucaGuarini]

@markwylde the pull request is on hold https://github.com/riot/riot/pull/1658