rniemeyer / knockout-postbox

A small library that uses Knockout's native pub/sub capabilities to facilitate decoupled communication between separate view models or components.
MIT License
350 stars 55 forks source link

ko.postbox.publish() creating duplicate updates #38

Closed wguldin closed 9 years ago

wguldin commented 9 years ago

Hi,

First off, this is a great plugin. I've got a question though, that I'm sure is something wrong on my end.

I've got a search box viewModel that publishes updates whenever the search box value changes (using textInput binding).

function searchViewModel() {
  this.searchText = ko.observable("")
     .extend({ rateLimit: { timeout: 200, method: "notifyWhenChangesStop" } })
     .publishOn("searchQuery");
}

ko.applyBindings(new searchViewModel(), document.getElementById('#search'));

This is then subscribed to by a listViewModel that does an AJAX request depending on if it is a query or a blank value.

ko.postbox.subscribe("searchQuery", function(newValue) {
    if(newValue !== '') {
        self.getContacts('?' + self.searchType() + '=' + newValue);
    } else {
        self.getContacts("");
    }
}, self);

This works wonderfully, except when I navigate to the url that is the list of results. /#/contacts (setup using Sammy:

this.get('#/contacts', function(context) {
    loadView('views/list/index.html', new listViewModel());
    ko.postbox.publish("searchQuery", "");

    loadPartialView('views/navigation/index.html', new navigationViewModel(), 'navigationContainer');
});

The first time, this works, but on subsequent navigations to this url (such as going back to it from a detail page), then the value is published twice, resulting in another db call. It's cumulative, too, the third time, two additional calls are made, and it just goes up from there.

Any advice would be much appreciated!

frederikprijck commented 9 years ago

Dear,

I saw this passing by and wanted to help you out. I am in no way any contributor to this library. But since the issue you are having is not related to postbox I thought I could help you out.

As far as I can tell, the value is not published twice. The subscription is registered again for each time the user navigates to ur page.

The reason for this is that in your sammyjs route, you create a new listViewModel, in this listViewModel you're subscribing for the correct message (searchQuery). So everytime you create a new listViewModel, you have a new subscription handler.

Let me explain it a bit with an example:

Possible solutions:

  1. Don't make a new instance everytime.
  2. Make sure to unregister the postbox registration before creating a new listViewModel (however, this still can result in other issues).
  3. Try using the delete keyword to remove your old instance.

If you have any questions, feel free to ask.

Apart from this, a small remark about your code:

this.get('#/contacts', function(context) {
    loadView('views/list/index.html', new listViewModel());
    ko.postbox.publish("searchQuery", "");

    loadPartialView('views/navigation/index.html', new navigationViewModel(), 'navigationContainer');
});

Why are you publishing an empy value each time the user navigates to the page ? Can't you just have an initial state for the listViewModel ? I find it very odd to see this kind of publish ... It's not that its a very bad thing, it just looks weird and unnecessary.

wguldin commented 9 years ago

Thank you so much for your help. I'm a web designer just trying to work my way through Knockout, so I really appreciate your help!

What you're saying makes sense. I'm using a code snippet from SO to load each list view that looks like this:

function loadView(url, viewModel) {

    var loadViewRequest = $.ajax({ 
        type: 'GET',
        url: url
    });

    loadViewRequest.done(function(response) {
        var container = $('#main');
        var view = container.find('.view');
        var newView = $('<div>').addClass('view').html(response);

        if (view.length) {
            ko.removeNode(view[0]); // Clean up previous view
        }

        container.append(newView);
        ko.applyBindings(viewModel, newView[0]);
    })
}

My understanding was that the ko.removeNode was doing the cleanup you mentioned, but it sounds like I'm mistaken. Here's the original SO about it.

Is the 'delete' keyword you mentioned a better way to handle this, instead of removeNode?

The idea of using this technique was that I could use partial views, and not have all my markup for this app on one page. That just felt not very clean, but this might not be any better.

As for publishing an empty string on the url being hit, I think I had to do this because the initial publish that is there by default didn't trigger my subscribe event in the other view model, for some reason.

Thanks for the advice!

frederikprijck commented 9 years ago

removeNode is knockout but your issue is a pure memory issue in javascript.

So each time a new listviewmodel is instantiated, you call removeNode before calling applyBindings. This is to tell the html which viewmodel it should use to bind to the view. To be more precise: you are removing replacing your html and viewmodel per request to the contacts. So each time the user comes on the contacts page, the old view and viewmodel is thrown away (out of the page, not out of the memory) and a new instance is created..

But the viewModel A is still existing in your memory, but modification will not be visualised in the view since it is not bound to any html (this is wat removeNode does: It clears the binding and removes the html but it does not remove the object from memory inmediatly). However, if you didn't unregister your knockoutpostbox, or any other "event handler" (even a button click etc) this will still be triggered when the corresponding event (postbox, clickhandler, ...) occurs.

Example can be found here: http://jsfiddle.net/j1d0h97g/

So again, your issue has nothing to do with knockout or knockout postbox. Using partial views is the way to go, but again... nothing to do with your issue.

One of the solutions could be to remove the subscriptions like so: http://jsfiddle.net/j1d0h97g/1/

To be more precise towards your example:

Example: http://jsfiddle.net/j1d0h97g/3/

Another extra remark: Have a look at John Papa's idea on SPA using KnockoutJS: http://www.johnpapa.net/building-single-page-apps-with-knockout-jquery-and-web-api-ndash-the-story-begins/

wguldin commented 9 years ago

Wow, that's super useful. I added underscore to the project and implemented the solution you outlined in your third fiddle. That's stopped the duplicate calls, and looks good.

Thanks a ton for your help!