inner mapping options not respected about knockout.mapping HOT 17 CLOSED

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

from knockout.mapping.

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?

from knockout.mapping.

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?

from knockout.mapping.

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!

from knockout.mapping.

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.

from knockout.mapping.

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.

from knockout.mapping.

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?

from knockout.mapping.

Of course, I'll get this done today.

from knockout.mapping.

Thanks a lot :)

from knockout.mapping.

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
[email protected]
wrote:

Thanks a lot :)

---

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

from knockout.mapping.

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

from knockout.mapping.

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.

from knockout.mapping.

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

On Tue, Feb 14, 2012 at 6:25 PM, James Foster
[email protected]
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)

from knockout.mapping.

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.

from knockout.mapping.

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.

from knockout.mapping.

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

from knockout.mapping.

I've reverted the fix for now.

from knockout.mapping.