Riot: Conditional tags are still fully evaluated

[swese44]

I have a use case where I want to render only one of several custom tags based on opts passed in. I have added an "if" conditional to each tag, assuming the conditional will be evaluated before the entire tag is evaluated/rendered, however all script logic in tags that should not be rendered is still executed.

    <customTag1 if={opts.data.type === 'customTag1'} data={opts.data} />
    <customTag2 if={opts.data.type === 'customTag2'} data={opts.data} /> 
    <customTag3 if={opts.data.type === 'customTag3'} data={opts.data} />

In this example, if opts.data.type is "customTag1", instances of customTag2 and customTag3 are still created and their script blocks are executed.

How can I conditionally include custom tags? Thanks!

[avimar]

I just did a quick test with similar code I have.
In riot 2.1, it runs each code base AND even the this.on('mount', callback) gets triggered!

I upgraded to 2.2.2 and the mount doesn't get triggered, but the code blocks still do.
I didn't catch this because I was mostly defining call-able functions, not something that ran on it's own.
I tried wrapping it in a div, e.g. <div if={LOGIC}><customTag1/></div> and I got the same results...

[swese44]

@avimar yes it's evaluating the entire tag tree. Fixing this should also improve performance.

[pandada8]

+1 experienced a lag of updating too

[paulcsmith]

This sounds serious. Is it verified yet?

[GianlucaGuarini]

Please check this example http://plnkr.co/edit/b0q5ZH8YcwwCHokZQS33?p=preview
riot evaluates the children tags but it executes the mount event only if the if expression is true.
At the moment the only way to avoid executing useless code is to wrap it in a mount callback.
We will change this in future evaluating the children tags only if it's really necessary

[paulcsmith]

Awesome, thanks for the example. That's a good workaround for now. Looking forward to it not evaluating until needed :D Thanks for the awesome work on Riot!

[swese44]

Putting all init logic in the mount event is not a good solution, it means Riot is creating an initial DOM and then immediately re-rendering. This is inefficient for deeply nested tag structures, in fact it creates so many child nodes that we get this error in riot.observable and replaceYield:
Uncaught RangeError: Maximum call stack size exceeded

We need to render a tree of child tags conditionally based on their specified tag type:

<child>
    <panel if={opts.data.type === 'panel'} data={opts.data} />
    <rotator if={opts.data.type === 'rotator'} data={opts.data} />
    <graphic if={opts.data.type === 'graphic'} data={opts.data} />
    etc. for each possible tag type
</child>

But if we try to do this, this bug causes scripts to be run for every single tag type for every single tag instance. We get JavaScript errors for every tag rendering the wrong tag type when data or this.* properties are missing.

In the meantime I've implemented this (also note that extra div wrappers are required or Riot will get confused about which tag to update):

<child>
    <div><panel each={this.getType('panel')} data={data} /></div>
    <div><rotator each={this.getType('rotator')} data={data} /></div>
    <div><graphic each={this.getType('graphic')} data={data} /></div>

    <script>
        this.getType = function(type) {
            var items = [];
            if (this.opts.data.type === type) {
                items.push({data: this.opts.data});
            }
            return items;
        };
    </script>
</child>

But this causes more problems -- whenever we use each all parent properties are copied down to the children and polluting every child in the tree.

What would it take to fix the bug now instead of waiting for a 2.3 roadmap? Thanks!

[gotno]

this is a problem for me as well with a component that needs to set up window scroll and resize hooks on('mount'), and triggers actions based on its location. the problem being that i also need to trigger those hooks on('update'), because the component could have moved.

maybe a simple fix for now could be to make it so child tags that are conditionally mounted with if= have their isMounted variable set to false so we at least have something to check within the child tag itself? right now that property doesn't change: http://jsfiddle.net/gotno/cj1cvb5q/2/

[clems71]

I have the same problem as @swese44 on 2.2.4 and it seems pretty serious. I get the 'mount' event fired even if the 'if' attribute evaluates to 'false'.

[rsbondi]

have a look at this example and see if it resolves the issue, you can fork to test other scenarios, this uses a wip version of riot attempting to resolve the issue.

cc: @ilblog

[jkurz]

@rsbondi your plunker does not unmount the element from what I can see. http://plnkr.co/edit/a5V4lYYZTJJWgaRx0Ezz?p=preview maybe that was not your goal. That is what I would expect in this situation. Maybe i'm wrong. I don't see the theoretical issue with doing this.

My biggest use case for this is cleaning up, any listeners in the block, killing intervals, things of this of this nature.

@gotno idea sounds like something that could work and would be simple to do. Just have an isMounted flag be false and then we can unmount ourselves if we want to from inside update callback.

Currently I do not see a way for us to conditionally rely on if to let us know when a component should be cleaned up.

[nathankot]

Currently I do not see a way for us to conditionally rely on if to let us know when a component should be cleaned up.

I second this. Currently I'm building a (child) component that uses canvas and makes repeated calls to window.requestAnimationFrame, there doesn't seem to be a way to know when to clean up the animation loop when the said component is removed using an if expression.

[avimar]

Just to state what I think is obvious, to make sure we're all on the same page:

Expected performance of if:

• When value is or changes to _truthy_: it evaluates the tag's script section and triggers mount.
• When value is _false_: do NOT evaluate the tag's script section, do not mount.
• When value _changes_ to _false_: hide and trigger unmount. (Hmm, hide or actually destroy the tag? I'm not totally sure of the difference.)

[nathankot]

@avimar that's what I had expected when using the if tag :+1:

When value changes to false: hide and trigger unmount. - in #1285 @GianlucaGuarini seems to have shot this down, but I don't 100% understand why firing the unmount event is undesirable? Legacy, semantic reasons?

[gotno]

@avimar that's what makes most sense to me, too :+1:

[clems71]

Agree with that :+1:

[BirdieMcFly]

Hi,
First of all, thanks a lot for your awesome work on RiotJS!!!
It appears clearly that riot would need a hard "if", the actual "if" behaving more like a show/hide.
We solved for now the problem using "each" with an empty array or an array with one element.

We swapped :
<my-tag if={myBoolean}></my-tag>
With :
<my-tag each={myBoolean ? [1]: []}></my-tag>

But it feels more like a hack and it is not 100% clear when we read that kind of code.
However we noticed that the actual "if" is also useful we have cases in which it makes sense.
What we would need really i think :

• a soft if that behaves like the actual if (mounts when if=true and doesn't unmount when if=false)
• a hard if that does what the hack with each does (mount AND unmount) but with an appropriate semantic

Actual "if" could be renamed "show" and new "if" could behave like the "each hack".
Or to preserve compatibility, "if" could stay as is and the new "if" would be given another name like "hardIf".

Any thoughts on that?

[clems71]

First, congrats for the awesome work on RiotJS with the latest releases.

What proposes @BirdieMcFly sounds reasonable to me. I like the solution as it allows us to keep the 2 wanted behaviors, and would not break existing code. So I'd say it's a win/win situation with this solution.

[nippur72]

+1

A better workaround is to create a custom if tag (it could be called include or else) like in this JsFiddle

<myapp>
</myapp>

<script type="riot/tag">
   <myapp>
      <if cond={true}>this shows because cond is true</if>
      <if cond={false}>this doesnt show</if>
   </myapp>
</script>

<script type="riot/tag">
   <if>
      <div each={opts.cond ? [1] : []}>
         <yield>
      </div>
   </if>
</script>

[BirdieMcFly]

@nippur72 that solution seems elegant but i have three problems with it :

• if cond is false you will still have an empty <if> tag in your DOM
• the conditioned content will have the <if> component as its direct parent, so if you want to pass datas in the content you have now to refer to the <if> parent. So for exemple you have a structure that is always here and in the future, the logic changes and you want to condition its presence, you have to rewrite the way datas are passed to that content (and now it's not the case with actual if and each).
• it forces the content to be nested in a <div>, which is not always what you'll want.

[rsbondi]

I like the each hack, that is similar to my hack for a with attribute

There already is a show attribute so I think we could replace the current if with the each hack implemented internally.

[rsbondi]

I was really hoping to use each internally to do this, it would automatically make <virtual> work with if. Unfortunately there is a conflict here with each and if on the same element(not sure I like that approach).

[willvedd]

+1

I'm happy I found this issue, I thought I was going crazy. I'm finding that this substantially impacts the performance of my applications. I'll look into the above workarounds but this either needs to be addressed in code or the the behavior more clearly defined in the docs.

[avimar]

Hmm, I was just thinking that if the issue is with the tag already mounting, then maybe we can do something like <if if={condition} riot-tag="my-tag"></if> -- is there a way to not evaluate and mount riot-tag attributes?

Also: confirming that <my-tag each={myBoolean ? [1]: []}></my-tag> works... but looks ugly. Is there not a way to use that same mechanism?

[RanzQ]

Is this the same problem as discussed here?

http://codepen.io/anon/pen/VedemG

I'd except all those 5 examples to render but only last one does with a hardcoded "true".

UPDATE: I'm using the each workaround for now. Thank you @BirdieMcFly!

[yannickoo]

<3

[GianlucaGuarini]

You can start testing these features installing npm install riot@next

[benstaker]

@BirdieMcFly thank you for your suggestions, I am currently applying these changes to our codebase.

An issue I've encountered though is with data references. Because each="" sticks the component in a child scope, the data references now require a parent. prefix. To resolve this, I have modified your suggestion to the following:

<my-tag each="{myBoolean ? [parent] : []}"></my-tag>

This now functions correctly.

UPDATE: Something weird is happening...

Let's take this example:
<my-tag if="{myBoolean}" reference="{parent.reference}"></my-tag>

Converting this to using the each="" method using [parent] requires the reference to remove the parent. prefix in order to work, like so:
<my-tag each="{myBoolean ? [parent] : []}" reference="{reference}"></my-tag>

...which is sticking it in the wrong scope.

However, if I revert back to using [1] it needs a parent. prefix attached to the reference (twice) to work, like so:
<my-tag each="{myBoolean ? [1] : []}" reference="{parent.parent.reference}"></my-tag>

I have a feeling this is due to the parent reference inside of the each="" attribute is based on the scope it's defined from, thus causing it to be applied to the parent.parent. scope. As far as I know, there isn't a way to reference the scope we're currently in. I have tried using [this] but that doesn't seem to be working, will report back with further information.