Skip to content

Changes to the valueAccessor for the foreach binding aren't picked up #16

Description

@papandreou

In my app I have a view where the foreach binding is used with an observable array that's replaced when the user navigates somewhere else. Something like this:

<div data-bind="foreach: state.foo().entries, selection: {data: selection}">

This construct works fine with foreach, but knockout.selection doesn't react to it when state.foo().entries evaluates to a different observableArray instance. It keeps working on the old observable array, leading to quite bizarre results.

As far as I can tell, supporting this would require knockout.selection to have an update handler that checks whether allBindingsAccessor().foreach.data has changed and updates its state accordingly. I've verified that update does fire under those circumstances.

This will probably require saving a lot of the state that's currently kept in variables inside init to be moved somewhere so it can be accessed from update.

I've added some failing tests here: https://github.com/papandreou/knockout.selection/tree/dynamic_foreach

Ping @sunesimonsen -- this doesn't appear to be addressed by https://github.com/sunesimonsen/knockout.selection/tree/refactoring yet. Is it something that you were planning to fix?

Activity

  1. sunesimonsen commented on Mar 31, 2013

    @sunesimonsen
    Collaborator

    @papandreou the refactoring branch is no longer relevant as it has been merged some time ago. I have delete the branch to avoid confusion in the future.

    About monitoring when the data observable of the foreach binding is updated, I think it would be possible to just subscribe to changes on the observable. Just remember to check the it is actually observable and also remember to dispose the subscription when the element is removed from the DOM.

    Something along the lines:

    var value = valueAccessor();
    if (ko.isObservable(value)) {
        var forEachSubscription = value.subscribe(function (data) {
            // Do stuff
        });
    
        ko.utils.domNodeDisposal.addDisposeCallback(element, function () {
            forEachSubscription.dispose();
        });
    }
    
  2. mwoc commented on Apr 1, 2013

    @mwoc
    Collaborator

    @papandreou Perhaps this could be solved in the same subscription you added in this commit:
    papandreou@0205083

    Something like:
    mwoc@2f39367

    It's tested with my production code in which the foreach can both be replaced completely (change to different data set), or only to some degree (sorting/filtering/pagination of same data set, in which some selected items may stay in the foreach and others are not). Have not been able to get my head around writing unit tests for this though, going to bed now.

  3. papandreou commented on Apr 1, 2013

    @papandreou
    CollaboratorAuthor

    @mwoc Unfortunately that subscription only fires when the contents of the previously bound observable array changes, but not at the moment when the foreach expression evaluates to a different observable array.

    Hopefully it's just a matter of subscribing to allBindingsAccessor and reinitialize if allBindingsAccessor().foreach evaluates to a different observable array instance like Sune suggested.

  4. sunesimonsen commented on Apr 1, 2013

    @sunesimonsen
    Collaborator

    @papandreou you are right, I of cause meant subscribing to the foreach variable in the ko.bindingHandlers.selection.init function - my bad.

  5. bramstein commented on Apr 3, 2013

    @bramstein
    Owner

    That indeed sounds like the right approach. @papandreou could you do a pull request for those failing tests. Would be nice to have the tests before we start working on the implementation.

  6. papandreou commented on Apr 3, 2013

    @papandreou
    CollaboratorAuthor

    @bramstein Absolutely: #25

  7. papandreou commented on Apr 10, 2013

    @papandreou
    CollaboratorAuthor

    I ended up working around the bug by wrapping my view in a with binding:

    <!--ko 'with': state.foo()-->
    <div data-bind="foreach: entries, selection: {...}">
       ...
    </div>
    <!--/ko-->

    Would still be nice to get this fixed, of course.

  8. added a commit that references this issue on Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions