improve clone/deepMixIn/deepFillIn behavior when dealing with custom instances

[millermedeiros]

see millermedeiros/amd-utils#116 and millermedeiros/amd-utils#117 for more info.

[satazor]

Worth mentioning that the instanceof is not the only problem. According to the current implementation, the cloned object will not have any members from the prototype. So calling functions in the cloned object would simply fail.

@millermedeiros your PR solves this but the implementation is not complete, because the own properties of the instance should also be cloned (arrays, objects, etc). Imagine this:

var Xpto = function () {
  this.obj = { prop: 'bar' };
  this.obj2 = this.obj;
};
var someXpto = new Xpto();
var cloned = clone(someXpto);
var otherCloned = clone(someXpto);

cloned.obj === otherCloned.obj; // This happens with the PR implementation

By cloning the own properties, it would avoid the issue above but could cause strange things to happen (e.g.: someXpto.obj !== someXpto.obj2 would be true).
I'm getting the feeling that we can't do much more about this.

[satazor]

Maybe we should rethink this. Should we really clone instances or delegate that responsibility to the user?
I suggested earlier something like:

function clone(prop, func)

Where func is a function to call when the clone function finds a custom type. It's not mandatory, and when not supplied it would have the current behaviour.

[satazor]

This also raises questions to the current deepMixIn and deepFillIn (and possibly others?) implementations, because when they find an object they deeply iterate over it. But instances are also objects so the problem is essentially the same.

In my opinion, deepMixIn and deepFillIn should stop the recursion when they find an instance.

[conradz]

I think that @satazor's solution is preferable over a simple boolean flag. We could create a function that behaves the way it would now when the flag is set, and then include that with the library (maybe called cloneInstance). deepMixIn and deepFillIn could behave the same as clone (accepting a function parameter), or could simply not clone the object. There are more choices than a simple boolean choice on how to clone instances.

[satazor]

@conradz The problem with deepFillIn and deepMixIn is that they accept multiple arguments. so we can't really do that without BC.

[satazor]

deepMixIn({}, { instance: new MyAwesomeConstructor()};

Is going to iterate on the instance enumerable properties and will return a completely unusable copy of the instance.
I think that deepMixIn and similar functions should stop iterating when they find an instance.

There is a lot of use cases for this. One of them is that you have a View in your application that has options so you deepMixIn your default options with the passed ones. But what if one of the options is actually an instance? For example an instance of a Model or another View?

var defaultOptions = { model: null };
function MyView(options) {
  this.options = deepMixIn(defaultOptions, options};
};

var person = new Person();
var view = new MyView({ model: person }); // Oops, view.options.model would be something strange

I know that the simpler mixIn wouldn't cause the model option to break, but there is cases in which you have options that have more than 1 level of nesting.

[conradz]

Yeah, I think that would probably be preferable.

[satazor]

@conradz updated my post above.

[millermedeiros]

it's hard to decide what is the best approach:

• lodash clone and deepClone only copies object own properties and doesn't do anything special about instances (copies as regular object - mout current behavior).
• underscore clone copies all properties (including from the prototype), underscore doesn't have a deepClone method.

I thought that both projects had the same behavior so that's why I said we should follow underscore. To me what makes more sense is lodash behavior (use forOwn) since when you are cloning natives the properties from the prototype are already inherited by default - no need to copy the values manually and in fact it will cause sync issues (updating Object.prototype won't propagate to cloned objects).

PS: mout/lang/clone is always a deep clone, maybe we should add another method for shallow clone or rename it to deepClone.

[satazor]

@millermedeiros That's why I think we should provide a default behavior but, ultimately, it would be the user to decide by passing a func.

[millermedeiros]

Maybe all the deep methods should only support 2 objects, that way we can add a 3rd argument to control how to behave on custom instances and how to merge arrays (maybe pass a config object instead of simply a callback)

[conradz]

Closed with #2