Fix/unify options merging

[DzenisevichK]

Fix for #39, #61, #62 issues

[sagacity]

Excellent work. Let me review the patch and I'll try to merge it asap.

[DzenisevichK]

Good!
There are also pending improvements but only after merging current request...

[sagacity]

Will you also be fixing the failing test (thanks for that too!) or should I handle it? Either is fine, I just want to know what your intentions are.

[DzenisevichK]

Yes, will fix in one hour.

[DzenisevichK]

Do we need export visitModel?
visitModel has the same abstraction level as updateViewModel and visitPropertiesOrArrayEntries...

The same also for getType that not related to ko.mapping stuff at all.

[sagacity]

I don't think anyone will miss visitModel, since it's undocumented.

I do want to export getType though, because I want to allow other types to be treated properly as wel. For instance, I'm using the XDate library in my codebase at work, and I want to be able to have getType return "date" for all XDate instances as well. So I need to be able to extend it. If you have an alternative solution to handle this that would be certainly be okay too.

[DzenisevichK]

It's enough for the request and I'm waiting for merging...

[sagacity]

Hmm, I'm seeing some issues. Occasionally the mapping options object itself (e.g. everything in __ko_mapping__) gets mapped as well! I haven't been able to reproduce this yet.

@DzenisevichK do you know what could be causing this behavior? It has to do with converting to JSON and then using that as a source to map again, I think.

[DzenisevichK]

May you create a unit test on jsfiddle to reproduce this?

[sagacity]

Yes, I'll try to do that.

[sagacity]

I have added a failing test. Basically, when using the create callback I would still expect that explicitly provided options like include get used. Instead they get overwritten at the start of visitModel.

Maybe it's not as clean when an include on the highest level is also used on child/nested objects though, so a different approach is also welcome.

[DzenisevichK]

Yes, this is not logically to use options for root object in nested!

Problem not in the pull request but in the options' notation at all. We need to create a new issue for discussion options' notation.

[sagacity]

@DzenisevichK Can you also take a look at issue #67?

[DzenisevichK]

Roy, please look on changes in my fork...
I'm also planning to implement js-traverse's way of transforming objects in ko.mapping library.

[sagacity]

Excellent work! It'll take some time for me to review everything, but it seems very nice.
Just curious, what would be the benefit of the js-traverse approach? (I'm not saying it is bad, just curious)

[DzenisevichK]

1. js-traverse approach is very flexible - not limited to any format of options - you may specify callback and based on mapping state as context of callback do any additional to wrap/unwrap transforming.
2. toJS and fromJS will be symmetric.
3. Don't save mapping state in object (reduce memory + also don't limit objects range).
   ...

[sagacity]

Sounds very interesting. I'm very pleased with the way you are reducing the code complexity, by the way!