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

Support keyed #each #81

Closed
Swatinem opened this issue Dec 1, 2016 · 2 comments
Closed

Support keyed #each #81

Swatinem opened this issue Dec 1, 2016 · 2 comments
Milestone

Comments

@Swatinem
Copy link
Member

Swatinem commented Dec 1, 2016

This would fix #77 along with all the other benefits that keyed iteration would bring.

What we definitely would need is a way to move fragments around in the DOM.

To achieve this, I would split the render (constructor) method into separate create and mount/insert methods.

  • create would do createElement etc.
  • mount (or insert, pending bikeshed) would actually mount the elements to the DOM. Instead of making a distinction for useAnchor, I would just always generate code such as parent.insertBefore( elem, anchor ), since insertBefore is the same as appendChild when anchor is undefined.

[this would also go in the direction I was thinking of in #37]

As for the #each itself, it would be as simple as iterating the list and create fragments that do not exist yet, then mount the fragment to the anchor, effectively moving already existing fragments. And then just unmount/teardown all the leftovers.

That should do as a first naive solution. Not moving existing fragments at all would require a more elaborate list diff, using lcs (longest common subsequence) and friends.

Any ideas on that @Rich-Harris?

@Rich-Harris
Copy link
Member

So the compiler would generate code something like this?

function renderSomeFragment ( root, component ) {
  var p = document.createElement( 'p' );
  p.appendChild( document.createTextNode( "Hello" ) );

  return {
    mount ( target, anchor ) {
      target.insertBefore( p, anchor );
    },
 
    update: noop,

    teardown: function ( detach ) {
      if ( detach ) p.parentNode.removeChild( p );
    }
  };
}

Yeah, if it brings us closer to keyed updates and doesn't have any negative effects on code size/performance (shouldn't, right? In fact it might make initial renders quicker since you could delay mounting top-level nodes until they were populated) then I'm all for it.

Might be easier to do that as a first step, without changing anything else, then come back to the keyed updates stuff.

On which: see also #33. To be honest we can probably get away with a fairly naive diffing process, since we're most often talking about single inserts/removals. I'll dig up the code we used for this problem in Ractive.

Since there's some overhead involved, I definitely think keyed updates should be opt-in, perhaps via some syntax in the {{#each ...}} block itself.

@AfraidKnot
Copy link

For the syntax, I would just extend the current syntax.
{{#each rows as row, y=row.id}}

If you need both then add another comma and a label for the idx variable.

{{#each rows as row, idx, y=row.id}}

or (not both)

{{#each rows as row, y=row.id, idx}}

Personal opinion: performance has a direct tie to user experience so it shouldn't be opt-in. I have no objection to an opt-out for those cases where it makes sense. Another possible solution is using another keyword ('for' for example) where one indicates it is keyed an the other is not.

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

No branches or pull requests

3 participants