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

Only update each-blocks if their value has changed #381

Closed
Rich-Harris opened this issue Mar 16, 2017 · 3 comments
Closed

Only update each-blocks if their value has changed #381

Rich-Harris opened this issue Mar 16, 2017 · 3 comments
Labels

Comments

@Rich-Harris
Copy link
Member

The 'Each blocks' example in the REPL produces this code:

// ...	
update: function ( changed, root ) {
    var __tmp;

    var eachBlock_value = root.cats;
    
    for ( var i = 0; i < eachBlock_value.length; i += 1 ) {
        if ( !eachBlock_iterations[i] ) {
            eachBlock_iterations[i] = renderEachBlock( root, eachBlock_value, eachBlock_value[i], i, component );
            eachBlock_iterations[i].mount( eachBlock_anchor.parentNode, eachBlock_anchor );
        } else {
            eachBlock_iterations[i].update( changed, root, eachBlock_value, eachBlock_value[i], i );
        }
    }
    
    teardownEach( eachBlock_iterations, true, eachBlock_value.length );
    
    eachBlock_iterations.length = eachBlock_value.length;
},

If it were this instead...

// ...	
update: function ( changed, root ) {
    var __tmp;

    if ( 'cats' in changed ) {
        var eachBlock_value = root.cats;
        
        for ( var i = 0; i < eachBlock_value.length; i += 1 ) {
            if ( !eachBlock_iterations[i] ) {
                eachBlock_iterations[i] = renderEachBlock( root, eachBlock_value, eachBlock_value[i], i, component );
                eachBlock_iterations[i].mount( eachBlock_anchor.parentNode, eachBlock_anchor );
            } else {
                eachBlock_iterations[i].update( changed, root, eachBlock_value, eachBlock_value[i], i );
            }
        }
        
        teardownEach( eachBlock_iterations, true, eachBlock_value.length );
        
        eachBlock_iterations.length = eachBlock_value.length;
    }
},

...we'd prevent a lot of unnecessary work.

@PaulBGD
Copy link
Member

PaulBGD commented Mar 16, 2017

I did this in some example code while working on #279, I must have missed it. Has changed always been in there?

@Rich-Harris
Copy link
Member Author

Honestly can't remember if it was there from the start. In any case, I realised it's slightly more complex than my example above makes out — while in this example...

<ul>
  {{#each cats as cat}}
    <li><a target='_blank' href='{{cat.video}}'>{{cat.name}}</a></li>
  {{/each}}
</ul>

...you can skip the block if cats wasn't one of the properties in the set(...) that triggered the update, that's not true if the each block contains data from outside that context:

{{#each comments as comment}}
  <div class='comment'>
    <span class='meta'>
      {{comment.author}} wrote {{elapsed(comment.time, time}} ago:
    </span>

    {{{comment.html}}}
  </div>
{{/each}}

In that example you'd need to update members of the block if time had changed (or elapsed, not that it would):

// ...	
update: function ( changed, root ) {
    var __tmp;

    if ( 'comments' in changed || 'time' in changed ) {
        var eachBlock_value = root.comments;
        
        for ( var i = 0; i < eachBlock_value.length; i += 1 ) {
            if ( !eachBlock_iterations[i] ) {
                eachBlock_iterations[i] = renderEachBlock( root, eachBlock_value, eachBlock_value[i], i, component );
                eachBlock_iterations[i].mount( eachBlock_anchor.parentNode, eachBlock_anchor );
            } else {
                eachBlock_iterations[i].update( changed, root, eachBlock_value, eachBlock_value[i], i );
            }
        }
        
        teardownEach( eachBlock_iterations, true, eachBlock_value.length );
        
        eachBlock_iterations.length = eachBlock_value.length;
    }
},

So we'd need to capture all the dependencies of any expressions used in the block. (We already get this for free in generator.contextualise, but it's not currently stored in the right way.)

Rich-Harris added a commit that referenced this issue Apr 17, 2017
Rich-Harris added a commit that referenced this issue Apr 17, 2017
Only update each blocks when their dependencies have changed
@Rich-Harris
Copy link
Member Author

Fixed in 1.16

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants