Comments (17)
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
Also, from the discussion above I would also change this line ...
... 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
Also, from the discussion above I would also change this line ...
... 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.
Related Issues (20)
- mapping fails if value = "constructor" HOT 2
- Is there a way to update the nested properties of the items of an ObservableArray? HOT 1
- Method visitPropertiesOrArrayEntries is doing unnecessary iterations on updateViewModel because the wrong variable is being used to check against ignored properties.
- Mapping not saved when data is an array HOT 1
- Mapping doesn't work when properties listed in different order HOT 4
- Empty observe array in mapping options creates observables from all properties HOT 1
- ko.mapping.toJSON to implement JSON formatting param like ko.toJSON
- Calling fromJS to amend an existing model with an ObservableArray fails
- NuGet package install directory
- Questions about Knockout Mapping HOT 10
- The "create" handler does not work with non-array properties HOT 1
- Create handler causes mapping to evaluate pureComputed HOT 1
- Suggestion: Safely update mapping when property in new object happens to be null
- mapping partial model into existing viewModel
- [Question] is arrayChanged part of the public api?
- objectLookup() + "watch" + Firefox = hilarity
- bug of optimizedKeys
- optionsCaption causes bound "value" to be deleted
- Doesn't seem to work with KO 3.4.2 HOT 1
- Having redudant data passed to an object makes it so the second reference of the same object is not updated
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from knockout.mapping.