Knockout: Regression in https://github.com/knockout/knockout/pull/2324

[bennieswart]

I've noticed a regression in https://github.com/knockout/knockout/pull/2324 where foreach (or template: { foreach }) creates two UI elements when there is only one element present in the array, duplicating the element.

I can confirm that this is the case, since inspecting the context of the binding at the time of malfunction shows only one element in the array, yet two elements are being displayed on screen.

I can also confirm that this is indeed the source of the regression, since building the branch prior to the merge works as expected.

Unfortunately I'm having a hard time reproducing this in a fiddle as I cannot seem to recreate the exact circumstances needed for the malfunction. In our code, however, the malfunction occurs with perfect reproducability.

At risk of biasing your diagnosis, I'll add that it seems like this happens when pushing into an empty ko.observableArray and then replacing the array with a new one. I'll continue investigating, seeing if I can create a simple example, but if the if the issue is apparent to anyone (especially @mbest, since he authored), please don't hesitate to take a shot at what it could be.

[brianmhunt]

@bennieswart if you want to try it, my knockout-fast-foreach plugin might yield different results that shed light on the issue.

[bennieswart]

Ok, I've narrowed it down to a simple example. Looks like this is a legitimate bug.

I've posted a minimal example at https://jsfiddle.net/53bvupxs/1/. You'll see the list contains only one element, but two are visible onscreen.

Unfortunately my expertise of knockout internals falls somewhat short here. Tracking down the exact cause will take me some time. I'm hoping one of the devs can identify this a bit quicker.

[chrisknoll]

So, I know nothing about what's going on inside these specific bindings. I agree there is some issue, but printing a duplicate is not it. Here's my observation on your test case, however:

This is the template:

  <!-- ko if: items().length -->
    <div data-bind="foreach: items">
      <div data-bind="text: $data"></div>
    </div>
  <!-- /ko -->
  <div data-bind="foreach: items">
  </div>

This chunk has 2 different databindings to bind to the items collection., however, in the first block, there is a inner template (for the foreach binding) while the second one has no template. The bug is that the second one is somehow using the first's template because the items are being printed for each of those foreach bindings. To confirm, i removed the second foreach binding to the second div, and then only one item was printed. So I agree with you about the bug, but the cause? I don't think it's a dupe printing, i think it is a template reuse issue.

[chrisknoll]

Update/Correction:

I inspected the DOM, and in fact both child divs are being palced under the first foreach databind. But it only does this with the presence of the second foreach binding. I still think there is some kind of cross-talk going on between those two elements..

In addition, the duplicaton appears using the form:

  <!-- ko foreach: items -->
  <!-- /ko -->

[mbest]

@bennieswart Thanks for the concise example. I'll work on tracking down the bug.

[bennieswart]

@chrisknoll that's exactly why I was having such a hard time creating a minimal example. It's not exactly your run-of-the-mill use case. Of course the example looks silly, but it's distilled from a much larger context.

@mbest thanks a lot!

[chrisknoll]

No complaints here. The simplest test-case is the best test-case.

[mbest]

I've narrowed this down to a problem with the arrayChange event that foreach now uses. Basically, it's possible to sometimes see a change event from the "past".

[fastfasterfastest]

Perhaps, perhaps not, related to another open foreach issue, https://github.com/knockout/knockout/issues/2305, where it is stated

... the descendants of foreach gets re-evaluated before they are removed and they are evaluated with values of $index (and $data) of the previous array

Regardless, perhaps a stone will kill two issues.

[mbest]

Fix in #2441.

[bennieswart]

Awesome, thanks!

[mbest]

@bennieswart Could you test this fix with your code?

[bennieswart]

@mbest before incorporating the fix, I see the strange behaviour. After incorporating it, I see the expected behaviour. It is only one case where I noticed the strange behaviour, but that case seems to be fixed now.

[mbest]

Thanks. I've merged the fix.

[fastfasterfastest]

FYI, it appears your stone killed https://github.com/knockout/knockout/issues/2305 as well - thanks!