grofit / knockout.merge

Adds basic merging functionality into knockout js mapping namespace
MIT License
16 stars 3 forks source link

More of question than issue: callbacks #10

Closed pzolla closed 9 years ago

pzolla commented 9 years ago

So I only kind-of know what I am doing. I believe I am running into issues with the merge not completing (consistently) before other functions are running. If this was my own function I would add in a callback to be returned when everything has completed. I poked around and tried a few things but am having trouble. Your code is a bit more complex than I am used to following. Any suggestions?

(By the way - this thing is awesome and solved a huge problem I was having - thanks!)

grofit commented 9 years ago

hmm well generally this would only occur if there were any async code, and generally the merge is synchronous. The only situation I could see where this would arise would be if there was a custom rule/method which did something which had async logic in there.

If you can post an example of your situation I can try to help, but unless you have introduced some async code then it should only finish execution once everything has been merged.

You can test this by just doing a ko.toJS(myModel); after the merge and seeing what is spat out.

pzolla commented 9 years ago

Oh yeah, I believe that is exactly my issue. I will try to be brief.

I am working on an HTML5 game that has a JS object 'world' representing the entire world state. For game 'Save' I save it as a JSON to a pouchDB database locally. I realized that when loading the world I was tripping over the sync nature of JavaScript. This was new to me (most of it is!). So I am using async.js to gain access to some sophisticated callback functionality. In this case, I am loading the world using async 'series' (one function at a time).

So in most cases I pass a callback to a function and return it to the series when finished. Works great. With KO mapping plugin, I do not pass a callback (I try not to screw with libraries directly). I think it works well because I am assigning it to a variable so JavaScript waits for the return in order to then process the variable.

With regards to knockout.merge, on 'load' I build the default world which is a mix of observables, non-observables, and computeds. I was having a hard time updating the 'world' object without either everything becoming observables, remapping everything manually, or breaking the computeds. So with your library, when I am ready, async.js runs merge which updates the default world with the saved JSON state. Works awesome! All the functionality remains intact.

With merge I did not pass the callback:

function (callback) {

    ko.merge.fromJS ( world, update);
    callback();

}

So really no managed delay. Would work great sometimes, other times not. I wrapped it in a setTimeout function and it works every time. So definitely a timing issue. I am leery about using setTImeout as I think the time needed will change as game gets larger. So I thought I would look at your code and see if I can pass a callback:

ko.merge.fromJS (world, update, callback)

But I will be honest. I really can't figure out where your code ends to "shove" the callback.

As I said, it is not my first choice to modify a library for the obvious reasons but in this case I was not sure what else to do.

Any thoughts would be appreciated.

Thanks

grofit commented 9 years ago

Well thats the thing, it ends synchronously by default, so if I was to do:

var someVm = new SomeVM();
var someData = { blah: "hello" };
ko.merge.fromJS(someVm, someData);
console.log(someVM); // blah would be set to "hello"

I mean if you needed to do callbacks you can easily just wrap it in one yourself like you show above:

var waitForMerge = function(callback) {
   ko.merge.fromJS(foo, bar);
   callback();
} 

But really as its synchronous it would offer no benefit.

In the case of you using asynchronous rules or methods within the merge then the merger will not really know about the callbacks there and will just assume once it finishes looping the model everything is complete. It is not a HUGE problem to re-write it to use callbacks but I assume most people use it synchronously so I would not want to break backwards compatibility. As if we were to introduce a callback then all rules and methods would need to handle the callbacks which would then require re-writes for existing users.

Just to confirm are you using any custom rules/methods with async code? if not this is all a moot point and it sounds like you have a race condition somewhere else.

For example a common use case is when people request data from a service via ajax like:

var somePerson = new SomePerson();
someHttpClient.Post("http://some-service/users/1234")
.then(function(personData) { ko.merge.fromJS(somePerson, personData); })
.then(itsAllDone)
.catch(function(error){ ... });

So in the above code lets pretend someone goes off and does an async call to a web server and gets some data back, the only point where that somePerson model will be populated is when itsAllDone is called. If you were to try to use somePerson on the lines after the Promise (the then/catch stuff) then it would not be set.

So in your example where you use some client side storage, I assume you do something like:

somePouchDBInstance.getDataWithKey("some-id", function(successData) {
   // do your merge here
});

If this still makes no sense or does not help if you can post a rough mockup of your code from where you start to get the data to where you merge it and it needs the callback I can try to help further.

pzolla commented 9 years ago

So this is very interesting.

I am not sure I followed the nuances of everything you wrote completely but my take away is this:

  1. You confirmed for me that your code is structured in a way that would require refactoring to make callbacks work, Hence my failure at a quick callback fix.
  2. You confirmed that:
var waitForMerge = function(callback) {
   ko.merge.fromJS(foo, bar);
   callback();
} 

is a synchronous approach with no benefit (which is what I stated earlier based on what I thought was happening. Now I have confirmation.)

That said, I think I figured this out with a lightly different approach. The async.js library recently (if I am reading the notes correctly) released a function called 'wrapSync' . This takes a sync function and makes it async, passing its return value to a callback.

Using their waterfall function (which rolls through functions passing values down), I wrapped merge in 'wrapSync'. If I am understanding what happens, 'wrapSync' is able to wait for merge to complete and then returns back to waterfall which then calls the callback,

async.waterfall( [
    async.wrapSync  ( ko.merge.fromJS( world, doc ) )
], next() );

Refreshing the browser 20 times, anecdotally, everything seems to work fine. I then dropped a function into your merge code that delayed its processing and console.logged a message hundreds of times. I saw no other function penetrate the console.log run. (I log a lot!). This leads me to believe the wrap worked as advertised.

So I think I am in good shape and if this is working as I believe, it is quite a neat tool to have figured out.

Thanks so much for your help.

grofit commented 9 years ago

if the async waterfall does what you need then go for it, if you have not already looked into it, I would recommend looking into promises, as async and others like it are good for kinda joining lots of async stuff together or trying to daisy chain stuff but in most cases Promises do it far better.

Like in the above example where you do the async.waterfall you would use that in a situation where you would normally use promises, as the problem being solved is "How can I chain together async tasks?" so the async library gives you waterfall, but promises (which are now part of the ES6 spec for JS, so are a standard structure) would do this:

Promise.resolve(something)
.then(doSomeOtherAsyncTask)
.then(function(returnFromPreviousStep){ return somethingElse(returnFromPreviousStep); })
.then(finallyDoACallbackHereOrWhatever)
.catch(function(err){ doSomethingWithError(err); });

You can even pass promises around so you could start a promise chain, then pass it to something else to continue the chain or whatever, very powerful stuff and it is a lot more elegant than the normal async handling.

pzolla commented 9 years ago

That's funny, they were next on my list. Since I never focused on async before I thought I would at least figure it out using callbacks (albeit with a library). I will now go back and see if I can implement a more elegant version of the code with Promises. Thanks again.