Ability to create missing observables on a merge

[IPWright83]

If you take an object like:

{ 
   title: ko.observable("A")
}

and merge it with an object like:

{
   title: "A",
   value: "a"
}

Then the newly created value field won't be observable. It would be great if there was a way to add additional items as observables.

Would you be open to having a flag to do this?

[grofit]

Just to confirm I get the scenario, you have a non observable model, and you want to merge in some observable data and have the output become observable? So kinda back to front of how you would normally use it? (currently I use knockout mapping for these sort of scenarios but depending on your answer I have more waffle on the subject).

[IPWright83]

No, the other way round. We've got a "placeholder" view model which has some values in it which is bound to the UI. I've using a ko.toJS call on the following POJO. The particular thing of interest here is the values array:

{
    "propertyType": "enum",
    "isAggregate": true,
    "view": "summary",  
    "name": "test",
    "id": "XX.XXX",
    "values": [
        { "text": "", "count": 0, "color": "#5fb5cc" },
        { "text": "", "count": 0, "color": "#75d181" },
        { "text": "", "count": 0, "color": "#f8a25b" }
    ],
    "aggregates": {
        "count": 0
    }
}

We're loading that onto a user interface, while we wait for an Ajax request to complete. What we get back will then be merged into the view model and might look like:

{
    "propertyType": "enum",
    "isAggregate": true,
    "view": "summary",  
    "name": "test",
    "id": "XX.XXX",
    "values": [
        { "text": "A", "count": 150, "color": "#5fb5cc" },
        { "text": "B", "count": 90, "color": "#75d181" },
        { "text": "C", "count": 75, "color": "#f8a25b" },
        { "text": "D", "count": 75, "color": "#ff0000" },
    ],
    "aggregates": {
        "count": 400
    }
}

At this point (with the array merging stuff I added) we end up with 3 items in the observable array, all which have observable properties. I also have a 4th item in the array but none of it's properties are observable.

What I'd like to be able to do is make that 4th value observable during the merge process.

[grofit]

Can this not be done with the withMergeConstructor or withMergeRule? as that way you can tell it specifically to convert those child objects to observables (or if they are typed automatically create a new instance and put it in the array).

I use this plugin for huge model trees of characters, so for example a character may have an inventory which has many items and a weight and some other stuff, so in that case I would assign the inventory.items = ko.observableArray().withMergeConstructor(Item); which would automatically create an Item for each entry in the array merged in, there is also a flag for if you should append or replace in arrays. Although in this case it sounds like you just want to append not replace.

If none of the above suits the scenario by all means we can discuss adding more functionality into the system to cater for it. I think the main problem is currently the knockout model is used as the reference and stuff is added into that as you traverse (with certain niche scenarios which differ), however with arrays and child objects you don't have a knockout model as reference so it wont know by default if it should create a new entry with all observables, or if just part of the dataset is needed etc. This then gets into the sort of scenario which some of the other mapping libs use where you end up having to define custom binding rules when you bind which often is not needed as 99% of most binding stuff can be inferred from the existing model.

Anyway let me know your thoughts on the above, and as always if you think there is an opening for more functionality do a rough fork/codepen or whatever and we can look at merging it in. (On a side note I have been thinking about making a more general es6 merge module which is not KO specific, but would probably have ko/aurelia/angular style extensions.)

[IPWright83]

I think I might be able to get away with some of the withMergeConstructor stuff, although I was hoping to not have to write any custom logic to do the merging ideally. I think actually in my clarification I made a mistake - I think I am creating the template view model manually, if you were doing it automatically with ko.fromJS then that approach wouldn't work.

I understand the need to stray away from lots of custom binding logic too, exactly what I want to avoid, which was why I was thinking of a general global (or possibly pass into merge) flag to control the behavior between POJO and observable's for just that merge.

I'll try sorting out a fork later on, and if you don't feel it fits I can always just keep using my fork instead.

[grofit]

Yeah, the only reason those methods exist is to cope with these sort of scenarios in an AOP style so the logic isnt within the models its outside, but if you do not use typed models it wont help much as you would need to manually append those rules to your newly created knockout stuff.

Thinking about it it sounds like you just want the default behavior of an array to create observables per entry, whereas I think (without looking) the default behavior is to ignore it unless you have some merge constructor. I am not sure if there are any problems with assuming the stance that any new array elements will have all properties created as observables...

Anyway have a think and let me know if you want the above behaviour and I will try to think a bit more on any issues that could arise from it.

[IPWright83]

The way I made it work arrays is that for any new object that is to be merged, it simply calls the exports.fromJS function passing in an empty object as the koModel. Here's a snippet where knockoutElement is an ObservableArray:

var placeholder = {};
exports.fromJS(placeholder, dataElement[i]);
knockoutElement.push(placeholder);

This is always going to fall into the final branch of exports.fromJS with an empty object, and simply do the POJO assignment:

else if (isEmptyObject) {
    koModel[parameter] = data[parameter];
}

I think we will want this sort of functionality, I don't really want to have to manually control merging. But I'm happy to create and maintain a fork if you don't want it in here. My gut reaction is there's some implied behavior in that assignment to an object, which someone might want to control so I wonder if anyone else has a similar use to case to myself.

[grofit]

Yeah, that scenario at the end was more for if you had something like:

function SomeType()
{
   this.Name = ko.observable();
   this.Age = ko.observable();

   this.SomeJSONData = {}; // Not observables but should be re-hydrated
}

Maybe the above is me just coupling my own scenario into the library, as I basically have models which allow plugins to write their own data into a metadata field which is not always going to be observables, it will probably mainly be JSON data but I want it to be added into the object. I assumed there may be some other fringe cases where people may want to do the same sort of thing (i.e have some non observable model data passed over into the VMs)

So the isEmptyObject is just there to find out if its an empty json object, if there is a better way to check for this scenario you may be able to have another end check, or even put a check before hand, like

//.. other handlers
else if(isArray(koModel)) {
   koModel[parameter] = ko.observable(data[parameter]);
}
else if (isEmptyObject) {
    koModel[parameter] = data[parameter];
}

[IPWright83]

I was thinking something similar... bearing in mind the object you're looking at isn't an array itself, it's just an item that needs to sit within the array. So something like:

else if(isEmptyObject && mergeMissingAsObservables) {
   koModel[parameter] = ko.observable(data[parameter]);
} else {
    koModel[parameter] = data[parameter];
}

[grofit]

the koModel should be an array shouldn't it? assuming koModel[parameter] is one of the elements?

If that is true then you can have it as a separate clause, as you would know you are within the context of an array, if not then like you have above you would need to have an options object passed in when merging. As you could do it as a global but then we are assuming each merge would require this behaviour. If we added the notion of arguments as an optional arg to the merging process you could pass in that stuff there and expose it to other stuff along the way, at least that way it keeps it kinda isolated.

[IPWright83]

No the koModel is the placeholder I mentioned, which gets initialized as an empty object. export.fromJS is actually copying the properties on one of the elements, rather than copying the elements themselves.

var placeholder = {};
exports.fromJS(placeholder, dataElement[i]);

When you're talking about passing parameters, are you thinking along these lines?

exports.fromJS = function (koModel, data, mergeMissingAsObservables) {
   // any future merges
  exports.fromJS(koModel[parameter], data[parameter], mergeMissingAsObservables);
}

[grofit]

oh right, I thought it passed in the containing array, wouldn't the placeholder be of type array?

Anyway that to one side I was thinking something along those lines but more future proof like:

export.fromJS = function(koModel, data, options) {
    // do stuff and pass options into child invocations
    else if(isEmptyObject && options.mergeMissingAsObservables) { blah(); }
}

This way if anyone makes custom rules/methods or global handlers they can have their own custom options which they can look out for.

[IPWright83]

Yeah, that's the sort of pattern I was thinking, so you specify the behavior at the high level merge call. I guess if you wanted to change the options for any reason at a different point in the stack we could allow it to be passed through the mergeConstructors etc.

Like the idea of sticking it under options.

[grofit]

If I get time later I will see if I can put something together for you to look over.

[IPWright83]

Cool, if not I'll see if I can look at it tomorrow. Here's a trace of my current use case showing how the object gets pushed through. Hopefully that makes things a little clearer:

[grofit]

Right I have added options to the system in 1.5.2 and have also added the discussed scenario as well as some tests for it. So let me know if it fixes your scenario, if not at least you can take a branch off 1.5.2 and add other optons/changes.

[IPWright83]

That's the right approach. Just had to make 1 small change as you'd missed a point passing the options through that happened to be the code we were exercising on line #47.

My colleague however has confirmed that works as expected with that additional options parameter though :) Thanks!

[grofit]

feel free to push a change up, otherwise I will get round to it later... happy merging.

[IPWright83]

I ended up making a few additional changes to make it always merge (even if the original view model isn't empty) and take recursion into account. Will try and merge at some point:

 else if (isPrimitive(data[parameter]) === false && options.mergeMissingAsObservables === true) {
                koModel[parameter] = {};
                exports.fromJS(koModel[parameter], data[parameter], options);
            }
            else {
                if (options.mergeMissingAsObservables === true) {
                    koModel[parameter] = ko.observable(data[parameter]);
                }
                else {
                    koModel[parameter] = data[parameter];
                }
            }

[grofit]

Sorry been quite busy, If you do finalize the change and the tests pass just commit up and I will accept the merge. Thanks again.