inner mapping options not respected #45

Closed
jamesfoster opened this Issue Jan 17, 2012 · 17 comments

Comments

Projects
None yet
3 participants
Contributor

jamesfoster commented Jan 17, 2012

See the following jsfiddle for an example of this problem.

http://jsfiddle.net/jamesfoster/HwGgH/

If I have the following object:

function Record(data) {
    var self = this;

    ko.mapping.fromJS(data, {
        copy: ['id']
    }, self);
}

when calling ko.mapping.toJSON(record) everything works as expected, but when the object is wrapped in some other context the id propery is missing

var viewModel = { record: new Record(...) };

ko.mapping.toJSON(viewModel) // id property is missing
ko.mapping.toJSON(viewModel.record) // works
Collaborator

RoyJacobs commented Feb 6, 2012

That's definitely odd. I'll take a look.

Contributor

jamesfoster commented Feb 6, 2012

my solution was to add the following code inside ko.mapping.visitModel:

        visitPropertiesOrArrayEntries(unwrappedRootObject, function (indexer) {
            var currentOptions = options;

            var mappingObject = unwrappedRootObject[mappingProperty];
            if(mappingObject) {
                currentOptions = ko.utils.extend({}, options); // clone
                currentOptions = ko.utils.extend(currentOptions, mappingObject);
            }

then use currentOptions instead of options. This way, if the object was mapped with fromJS, the options passed in will used when unmapping.

I'm unsure about the clone tho... Should it just use the original options?

Collaborator

RoyJacobs commented Feb 6, 2012

That looks good, but I have one question: Why are you merging the currentOptions with the original options instead of just taking them directly from mappingObject?

Contributor

jamesfoster commented Feb 6, 2012

That was my question at the end there. Merging seemed in keeping with how it works currently. I would actually prefer it to take the options directly from the mapping object!

Collaborator

RoyJacobs commented Feb 6, 2012

I'm sorry, I thought you were explicitly asking about why you needed to clone. Merging of options is currently only used when you provide options to ko.mapping.fromJS with an already mapped object. Doing a ko.mapping.toJS shouldn't merge any options, I don't think.

Can you maybe elaborate a bit on why you thought this was the case? Perhaps I'm overlooking something.

Contributor

bryansquared commented Feb 13, 2012

I just did:

                    var currentOptions = options;
                    var mappingObject = unwrappedRootObject[mappingProperty];
                    if (mappingObject) {
                        currentOptions = mappingObject;
                    }

and substituted every reference to options in the method with currentOptions except for these:

 options.parentName = getPropertyName(parentName, unwrappedRootObject, indexer);
var previouslyMappedValue = options.visitedObjects.get(propertyValue);
mappedRootObject[indexer] = callback(propertyValue, options.parentName);

And that seems to work for me.

Collaborator

RoyJacobs commented Feb 14, 2012

Would you be willing to create a pull request with the changes to knockout.mapping.js and make sure all the unit tests still pass?

Contributor

bryansquared commented Feb 14, 2012

Of course, I'll get this done today.

bryansquared added a commit to bryansquared/knockout.mapping that referenced this issue Feb 14, 2012

Addressed Issue #45, added 2 test cases.
Inner mapping options were not being respected
Collaborator

RoyJacobs commented Feb 14, 2012

Thanks a lot :)

@RoyJacobs RoyJacobs closed this Feb 14, 2012

Contributor

bryansquared commented Feb 14, 2012

I'm still very new to the knockout.mapping codebase so even if it
passes test cases it might be a good idea to give it a once over,
doesn't look very complicated though.

On Tue, Feb 14, 2012 at 11:18 AM, Roy Jacobs
reply@reply.github.com
wrote:

Thanks a lot :)


Reply to this email directly or view it on GitHub:
#45 (comment)

Collaborator

RoyJacobs commented Feb 14, 2012

It seems fine, as far as I can tell. @jamesfoster can you confirm?

Contributor

jamesfoster commented Feb 14, 2012

No. This solution is not correct! The inner options should cascade down.

I apologise for not replying earlier. Been rather busy.

So... change this line to pass in currentOptions

https://github.com/bryansquared/knockout.mapping/blob/ba783435d322276ab91b2cbcef60512bef11ddbc/knockout.mapping.js#L646

Also, from the discussion above I would also change this line ...

https://github.com/bryansquared/knockout.mapping/blob/ba783435d322276ab91b2cbcef60512bef11ddbc/knockout.mapping.js#L621

... to currentOptions = fillOptions(mappingObject); so that we aren't merging options from the parent context!

Here's a test to prove cascading.

test('inner mapping options should cascade down', function() {
  var obj = {
    name: "Lorem ipsum",
    items: [{name: 'soup', type:{name: 'food'}},{name: 'rock', type:{name: 'mineral'}}]
  };

  var mapping = {
    ignore: ['name'],
    items: {
      create: function(options) {
        return ko.mapping.fromJS(options.data, {copy: ['name']});
      }
    }
  }

  a = ko.mapping.fromJS(obj, mapping);
  b = ko.mapping.toJS(a);

  equal(a.name, undefined);
  equal(a.items()[0].name,'soup');
  equal(a.items()[0].type().name,'food');
  equal(a.items()[1].name,'rock');
  equal(a.items()[1].type().name,'mineral');
  equal(b.items[0].name,'soup');
  equal(b.items[0].type.name,'food');
  equal(b.items[1].name,'rock');
  equal(b.items[1].type.name,'mineral');
});

Currently, assertion 7 and 9 will fail because it will be ignoring the name property on grandchildren.

Contributor

bryansquared commented Feb 15, 2012

Excellent, I'll make these changes now and submit another pull request

On Tue, Feb 14, 2012 at 6:25 PM, James Foster
reply@reply.github.com
wrote:

No. This solution is not correct! The inner options should cascade down.

I apologise for not replying earlier. Been rather busy.

So... change this line to pass in currentOptions

https://github.com/bryansquared/knockout.mapping/blob/ba783435d322276ab91b2cbcef60512bef11ddbc/knockout.mapping.js#L646

Also, from the discussion above I would also change this line ...

https://github.com/bryansquared/knockout.mapping/blob/ba783435d322276ab91b2cbcef60512bef11ddbc/knockout.mapping.js#L621

... to currentOptions = fillOptions(mappingObject); so that we aren't merging options from the parent context!

Here's a test to prove cascading.

test('inner mapping options should cascade down', function() {
 var obj = {
   name: "Lorem ipsum",
   items: [{name: 'soup', type:{name: 'food'}},{name: 'rock', type:{name: 'mineral'}}]
 };

 var mapping = {
   ignore: ['name'],
   items: {
     create: function(options) {
       return ko.mapping.fromJS(options.data, {copy: ['name']});
     }
   }
 }

 a = ko.mapping.fromJS(obj, mapping);
 b = ko.mapping.toJS(a);

 equal(a.name, undefined);
 equal(a.items()[0].name,'soup');
 equal(a.items()[0].type().name,'food');
 equal(a.items()[1].name,'rock');
 equal(a.items()[1].type().name,'mineral');
 equal(b.items[0].name,'soup');
 equal(b.items[0].type.name,'food');
 equal(b.items[1].name,'rock');
 equal(b.items[1].type.name,'mineral');
});

Currently, assertion 7 and 9 will fail because it will be ignoring the name property on grandchildren.


Reply to this email directly or view it on GitHub:
#45 (comment)

Contributor

bryansquared commented Feb 15, 2012

Implementing these changes had a few unintended consequences:

Mapping: ko.mapping.fromJS should copy specified single property, also when going back .toJS, except when overridden
Mapping: ko.mapping.toJS should include specified single property
Mapping: ko.mapping.toJS should merge the default includes
Mapping: ko.mapping.toJS should merge the default ignores

If you would please advise, I will try to finish the changes.

Contributor

jamesfoster commented Feb 15, 2012

Ok. this is due to not copying the options from the parent. Which I believe is the correct approach, do you agree @RoyJacobs?

The way to solve this is to pass the overridding options into visitModel and apply them at every level of the hierarchy

Line 131:

        // Merge in the options used in fromJS
        options = fillOptions(options, rootObject[mappingProperty]);

        // We just unwrap everything at every level in the object graph
        return ko.mapping.visitModel(rootObject, function (x) {
            return ko.utils.unwrapObservable(x)
        }, options);

        // Clone the options used in fromJS
        var newOptions = fillOptions({}, rootObject[mappingProperty]);

        // We just unwrap everything at every level in the object graph
        return ko.mapping.visitModel(rootObject, function (x) {
            return ko.utils.unwrapObservable(x)
        }, newOptions, options);

Line 596:

    ko.mapping.visitModel = function (rootObject, callback, options) {

    ko.mapping.visitModel = function (rootObject, callback, options, overridingOptions) {

Line 620:

            if (mappingObject) {
                currentOptions = fillOptions(mappingObject,options);
            }

            if (mappingObject) {
                currentOptions = ko.utils.extend({}, overridingOptions);
                currentOptions = fillOptions(currentOptions, mappingObject);
            }

The clone here is necessary to ensure overridingOptions is unchanged. Note that fillOptions will alter the first argument.

Hopefully this solves the issue.

Collaborator

RoyJacobs commented Feb 15, 2012

The options that are specified at the root level should be passed along, yes. @bryansquared is James' explanation sufficient for you?

RoyJacobs added a commit that referenced this issue Feb 16, 2012

Collaborator

RoyJacobs commented Feb 16, 2012

I've reverted the fix for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment