Templating fixes #144

Merged
merged 8 commits into from Jun 6, 2012

Conversation

Projects
None yet
4 participants
Contributor

mbest commented May 12, 2012

It turns out that the behavior of the dummy templating engine in the templating tests hides some issues with memoization and templating output.

The main issue is that the default templating engine returns an array of node objects at the base level of the template, while the dummy templating engine wraps the template output in a single div. This means that (unlike with the default engine) there are never any data-bind memo comments at the base level of the template. This situation causes problems, so it shouldn't be suppressed in the tests.

When the memos are unmemoized, they aren't removed from the array of node objects when they're removed from their parent, so empty comment nodes end up in the DOM. This can cause nasty issues when combined with other features of Knockout:

  • When you use the beforeRemove hook for the template binding and remove the DOM node yourself, the empty comment nodes start to build up in the container, never getting removed.
  • When the first node is an empty comment node, it gets used as the node that triggers disposal of the dependent observable for the template when it's removed. This means that if you try to clean up the empty comment nodes yourself at a later time, your rendered templates stop updating.

This pull request makes a few changes:

  • Changes the templating tests to not wrap their output in a div and return it as an array, just like the default templating engine does. This reveals the empty comment node issues in the tests.
  • Fixes the issue for most templating by shifting the unmemoize step to be after the rendered nodes are added to the DOM.
  • Adds a test to catch empty comments that are still present when using the template binding with foreach (since the "ignoreTargetNode" rendering mode is used here).
  • Fixes the issue for this case by adding a memoize method that removes empty memos, and uses it after the foreach templates are all done rendering.
Contributor

SteveSanderson commented Aug 26, 2011

Thanks very much for this. I definitely want to apply these changes, but I'll be applying them to the 1.3 branch where there have been various changes to templating and binding logic already. So, it might take a little while before I can migrate your changes over to the new 1.3 world. Hope that's OK!

Contributor

seanami commented Aug 26, 2011

Not a problem. I have a workaround already in place (adding an empty memo cleaning method to the afterRender callback). I just wanted to get this in so you could see it asap.

I think the two most important things to get over into 1.3 are in the test changes:

  • Altering the dummy templating engine to not wrap output so it doesn't hide empty memo issues.
  • Adding a test for the foreach with data-bind on root template elements case, to ensure empty memos are removed there too.

Thanks for Knockout! I'm really enjoying using it. :)

Contributor

SteveSanderson commented Oct 11, 2011

Scheduling for consideration for v1.3.1 (as it's important, but not urgent enough to block 1.3.0)

Owner

mbest commented Apr 20, 2012

It appears that this bug, in conjunction with other changes in 2.1, causes the problem demonstrated here: http://jsfiddle.net/madcapnmckay/radm8/ I think we should fix this in 2.1 before it's released.

Owner

mbest commented Apr 20, 2012

Never mind. The problem in that fiddle is related, but not the same as this issue. Actually the main problem described here is already fixed in 2.0, which runs the unmemoize function after the nodes are added to the document. The problem demonstrated in the fiddle is that the memo comment is still in the array of nodes held by setDomNodeChildrenFromArrayMapping and if that comment is the first item in the array, it messes things up.

Contributor

SteveSanderson commented Apr 25, 2012

After seeing issue #440, which may not have occurred (or could have been detected by specs already) if this pull request had been merged already, I'm especially keen to include this in v2.2.

Also, reading through it again, it's an extremely well written pull request with the changes clearly delineated and justified - so thanks again Seanami!

Until we do merge this pull request in, it's not possible to write a spec to demonstrate the fix for #440 (because the dummy template engine doesn't experience the same issue).

Contributor

seanami commented Apr 25, 2012

You're welcome! :)

Owner

mbest commented Apr 27, 2012

To make sure arrayToDomNodeChildren doesn't try to track the removed comment, add this to the top of fixUpVirtualElements:

// Remove any initial nodes that aren't in the document
while (contiguousNodeArray.length && !ko.utils.domNodeIsAttachedToDocument(contiguousNodeArray[0]))
    contiguousNodeArray.splice(0, 1);

Sean McBride and others added some commits Aug 25, 2011

@mbest Sean McBride Alter templating tests to behave like real templating engines
The default templating engine returns an array of nodes that's not wrapped in
a container, and isn't a nodelist. The dummy templating engine in the test
always wraps the returned markup in a div, which masks some issues with the
templating code with empty memo comments that stick around in the markup.

After this change, all of the tests that are failing fail because of these
empty memo comments that stick around with the real templating engine due to
the templating result being returned as a bare array of nodes (which the memo
cleanup can't remove a node from).
ed307bc
@mbest Sean McBride Use a better util function in the templating tests a6a411d
@mbest mbest Fix small issues with merging template spec fixes by @seanami
* change makeArray to arrayPushAll (makeArray isn't exported)
* remove `div` wrapper from new tests (that were added since the original commit)
* fix other tests that were changed since the original commit
76ab9ea
Owner

mbest commented May 12, 2012

I've updated the template spec portion of this pull request to the latest version. I will add commits to include a new test and updated fix for #440.

@mbest mbest #440 and #144 (foreach, re-written template, data-bind)
Add spec to show that foreach correctly handles templates where first node has a binding.
Add secondary fix for #440, which will solve the original bug in a different way and possibly prevent other problems down the road.
98ffbd0

mbest referenced this pull request May 14, 2012

Closed

2.2 release discussion #479

@SteveSanderson SteveSanderson Fix spec failure that occurred only when *all* specs are run (not whe…
…n just running template specs).

Really, we should stop using `testNode` as a global variable, as it introduces order dependencies between specs.
c666cb5
Contributor

SteveSanderson commented May 18, 2012

Added a fix for a spec issue.

I'm a bit unsure about the following change:

// Remove any initial nodes that aren't in the document
while (contiguousNodeArray.length && !ko.utils.domNodeIsAttachedToDocument(contiguousNodeArray[0]))
    contiguousNodeArray.splice(0, 1);

Could you clarify why this is the right thing to do? It looks very special-casey, but the comment doesn't say what special case it addresses. Why are we only removing a continuous sequence of leading nodes, and not all nodes that aren't in the document? And if the fix to #440 already provides the desired behaviour, what extra behavior or safety does this subsequent addition provide?

Hope that doesn't sound too demanding - I just want to be sure all the code in KO will make sense to anyone who tries to understand it, and right now, I genuinely don't know what scenario this bit uniquely handles.

Contributor

SteveSanderson commented May 18, 2012

Also merged master into this branch so that Travis will verify it properly.

Owner

mbest commented May 18, 2012

Hope that doesn't sound too demanding.

Not at all. I'll work on it.

Owner

mbest commented May 18, 2012

Here's the explanation. If the first node in a string-based template has a binding, a comment is inserted before it during template rendering and then removed during un-memoization. But arrayToDomNodeChildren still includes that node in its mappedNodes array. When it needs to remove an array entry, it calls fixUpVirtualElements, but it doesn't do anything because the first node's nextSibling is null. Thus it fails to remove nodes that were added during binding or unmemoization.

I've created a fiddle to demonstrate: http://jsfiddle.net/mbest/KDGP9/ If you remove an item from the array, the x's will not be removed.

Now here's the same example with some text added before: http://jsfiddle.net/mbest/v8Ls6/ Now you'll see it working because the first node is okay.

Owner

mbest commented May 18, 2012

Why are we only removing a continuous sequence of leading nodes, and not all nodes that aren't in the document?

To answer this question, once we have an initial node in the document, the remainder will be have to be in the document since they are loaded using nextSibling. So we only need to make sure we have a valid initial node.

With this change we could revert the change made in #440 if we want.

Owner

mbest commented May 18, 2012

I've created a fiddle to demonstrate:

I first tried demonstrate the problem using an inner template with virtual elements (as that's what fixUpVirtualElements was designed to deal with), but I ran into a different bug that prevented it from working. Here's the fiddle and the issue: #494.

@mbest mbest #144 - arrayToDomNodeChildren/fixUpVirtualElements updates and specs
Add spec and expand comments on earlier change.
Fix up node list for any array with 2 or more items (rather than 3 or more).
Add spec for above.
3ec0059
Owner

mbest commented May 19, 2012

I added two new specs related to fixUpVirtualElements. One was fixed in 98ffbd0 and the other I've fixed in 3ec0059.

mbest was assigned May 31, 2012

Contributor

SteveSanderson commented Jun 6, 2012

Here's the explanation. If the first node in a string-based template has a binding, a comment is inserted before it during template rendering and then removed during un-memoization.

Thanks. I was aware of that, but my concern was that the code was diverging from its stated design and was introducing implicit dependencies that no longer quite made sense. Having the function fixUpVirtualElements become entangled with the implementation details of string-based templating, a totally separate feature, was an alarm for me.

In the end, I can accept that fixUpVirtualElements just has to make a bunch of guesses about what DOM nodes you are trying to remove/replace, but we should be explicit about this. So, I've renamed it to fixUpNodesToBeRemoved and updated the comments to clarify why we're doing this heuristic kind of thing.

Thanks very much for adding the extra specs, and for the extra bugfix. This is ready to merge now if you're happy with my function rename and the new comments. Or I'll merge it myself if you prefer - just let me know if so!

Owner

mbest commented Jun 6, 2012

This is good. Thanks for making the code more informative.

I think we may want to change the name again soon, though, as it will be used for more than determining nodes to remove (see #259, #484).

mbest merged commit c558d08 into master Jun 6, 2012

Agh, why is the Knockout release cycle so slow. I've been hitting my head against this problem for a few days before finally tracking it down to this line. I wish it were released already :(

@smerchek smerchek added a commit to softek/knockout that referenced this pull request Sep 2, 2014

@smerchek smerchek Fix error in fixUpContinuousNodeArray when using jquery.tmpl
The `shift` method does not exist on jQuery objects. However, the `splice` method does exist. Previously, `splice(0,1)` was used as of #144 but was replaced with `shift()` in [this commit](knockout@7c99a94#diff-2b4ca49d4bb0a774c4d4c1672d7aa781R235).

A workaround can be found [on StackOverflow:](http://stackoverflow.com/questions/6515544/how-to-pop-or-shift-from-jquery-set)

```
(function( $ ) {
    $.fn.pop = function() {
        var top = this.get(-1);
        this.splice(this.length-1,1);
        return top;
    };

    $.fn.shift = function() {
        var bottom = this.get(0);
        this.splice(0,1);
        return bottom;
    };
})( jQuery );
```
8b3c402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment