Computeds that return plain arrays don't report array changes

[AdamWillden]

Need fix for failing test in PR #10

[AdamWillden]

need to use $rawData in the fastForEach constructor. Would that option not always be the desired option anyway?

[brianmhunt]

Yes, I think you're absolutely right about always using $rawData - I think that solves other issues I was hitting too.

[AdamWillden]

@brianmhunt I see you fixed the failing test (though you didn't include it?)

I'm still having issues in my application and I think it's around the same issue despite my additional test now passing (I took the latest changes and rebased the test I wrote to check it passed).

This is the code that's causing me issue, it just shows 10 closest pages that the user can click on, be it 5 previous and 5 next pages or if for example the user is on page three it shows the previous two pages and the next 7. The result simply isn't updating like it did with foreach:

 this.previousPages = ko.pureComputed({
            'owner' : this,
            'deferEvaluation' : true,
            'read' : function() {
                var start;

                if (this.nextPages().length < 5) {
                    start = Math.max(1, this.pageIndex() - 10 + this.nextPages().length);
                } else {
                    start = Math.max(1, this.pageIndex() - 5);
                }

                var finish = Math.max(1, this.pageIndex());
                return _.range(start, finish);
            }
        });

[brianmhunt]

Thanks Adam - the test from your PR should be at the bottom of the spec (sorry I'm on mobile so it's slow!). If not I have missed it and need to add it.

Any chance you could post the output of page.subscribe(console.log, console) in a new issue where page is the observable argument for the foreach? That should be enough to go on and much appreciated!

I'll look too later today when I get to a computer.

Cheers

[AdamWillden]

Ah my apologies I obviously had my eyes closed looking at your commit for the unit test.

Just in the car atm and will post in the next few hours.

[AdamWillden]

Being previous pages I started off on page one and selected incrementing pages til page 4, here's the output:

[]
knockout-3.3.0-alpha-anim.js:1103 [1]
knockout-3.3.0-alpha-anim.js:1103 [1, 2]
knockout-3.3.0-alpha-anim.js:1103 [1, 2, 3]

Similarly I have nextPages which uses the following code:

        this.nextPages = ko.pureComputed({
            'owner' : this,
            'deferEvaluation' : true,
            'read' : function() {
                var maxPage = Math.max(10 - this.pageIndex(), 5);

                var start = Math.min(this.pageIndex(), this.maxPageIndex()) + 1;
                var finish = Math.min(this.pageIndex() + maxPage, this.maxPageIndex()) + 1;
                return _.range(start, finish);
            }
        });

And outputs:

[2, 3, 4, 5, 6, 7, 8, 9, 10]
knockout-3.3.0-alpha-anim.js:1103 [3, 4, 5, 6, 7, 8, 9, 10]
knockout-3.3.0-alpha-anim.js:1103 [4, 5, 6, 7, 8, 9, 10]
knockout-3.3.0-alpha-anim.js:1103 [5, 6, 7, 8, 9, 10]

Neither of these two computed observables show updates

[brianmhunt]

Got it - should be fixed now. I think I've got every reasonable possible permutation accounted for in the unit tests now (fingers crossed!).

Cheers

[AdamWillden]

@brianmhunt I was so ready to say "all systems go" but I'm still experiencing the same issue :-(

I'll try and debug to find where things aren't working!

[AdamWillden]

Right this seems to be partly my own app code. I've changed two things that have shown improvement and a separate issue.

To explain: nextPages and previousPages used to be functions and at some point I changed them to computeds and forgot to change a couple of extra things. Note however that this used to work with the foreach binding.

Previously I had this in my html markup: <!-- ko fastForEach: nextPages() -->. I assume in this case the previous foreach might have wrapped the binding valueAccessor in another computed or something like that to detect changes? I changed my markup to <!-- ko fastForEach: nextPages --> although this didn't immediately resolve things.

Second thing I had was a _.bindAll('nextPages', 'previousPages'). Why this would make a difference I really don't know but removing that bindAll fixed the issue at hand and I saw things updating.

However I'm facing another issue when deleting items: Failed to execute 'removeChild' on 'Node': This node type does not support this method. on this line this.element.removeChild((ptr && ptr.nextSibling) || this.element.firstChild)

[brianmhunt]

Thanks for your patience and persistence, @AdamWillden !

Any chance you could post the observable data values that lead to the Failed to execute? (i.e. with the nextPages.subscribe(console.log, console) – that would narrow down the issue.

Many thanks & Cheers

[AdamWillden]

I think you deserve a thanks for your patience! Its the same output as I put above but I've done it again to be sure

Selected page 2:

[2, 3, 4, 5, 6, 7, 8, 9, 10]
knockout-3.3.0-alpha-anim.js:1103 [3, 4, 5, 6, 7, 8, 9, 10] *exception occurs*

Reloaded the page and selected page 5

[2, 3, 4, 5, 6, 7, 8, 9, 10]
knockout-3.3.0-alpha-anim.js:1103 [6, 7, 8, 9, 10] *exception occurs*

[brianmhunt]

It's my pleasure! Hmm. I can't seem to reproduce the issue you are having now though.

Do you have any suggestions for permutations I might add to the test here that would reproduce the problem at hand?

https://github.com/brianmhunt/knockout-fast-foreach/blob/1f515960e482b2e887ea8f7fbd5851cdf6877873/spec/knockout-fast-foreach-spec.js#L213

What do the nodes being reproduced look like?

Cheers

[AdamWillden]

I'm just debugging a bit now... the html markup is the following (nothing strange):

<!-- ko fastForEach: previousPages -->
    <button data-bind="click: $parent.pageIndex, attr: { title: 'Go to page ' + $data.toString() }, text: $data"></button>
<!-- /ko -->

[brianmhunt]

Does the issue occur when you use a non-virtual element i.e. change <!-- ko ... --> to <div data-bind='..'>?

[AdamWillden]

Yeah I was just thinking that seems to be that you can't call removeChild on a comment node. I'll just test and report back

[AdamWillden]

@brianmhunt yeah that's the reason, works fine on non-virtual elements

[brianmhunt]

Got it - thanks Adam. Hoping to look at and fix it up tonight.

[AdamWillden]

EDIT - Ignore this post I might be a tool and getting confused. I don't know...

I'm seeing this bug more frequently now in other parts of my application (where I also use virtual elements).

Another bug in my own code was causing some entries to not get deleted meaning I didn't see this bug as often as I'd expect!! Gotta love coding...

[brianmhunt]

@AdamWillden – There was definitely an issue deleting elements from a virtual element list. Should be fixed now in 0.2.8 - but please let me know if you continue to encounter issues.

Cheers!

[AdamWillden]

@brianmhunt very happy to say that I am no longer experiencing any issues! Anything crops up I'll be sure to let you know in a new issue :+1: great work

[brianmhunt]

Awesome, thanks Adam. I've opened #13 incidentally; it won't work with the minified version of knockout, but the debug version will. It's not a hard fix, but is a nuisance - hope to get it in tonight / tomorrow.