Add instanceClone parameter to lang/clone

[conradz]

See #1.

This adds an instanceClone parameter to lang/clone, which allows passing a function that will be called to clone any non-native instance. A non-native instance is an object where .constructor is not Object.

Opening this for more feedback. Is this the way it should behave?

[satazor]

I think that if no func is passed, it should assume a custom one that does what underscore does which is create a new prototype and copy all the properties of the object as well as the prototype ones.

This would fill @millermedeiros requirement in following underscore behaviour by default.

[conradz]

OK, updated it to copy all properties on the prototype also. Would it be good to also have a function to clone an object, keeping the prototype? Maybe call it object/cloneInstance? It could then be passed to the clone function to keep existing prototypes.

[satazor]

@conradz yes I think it would be useful to have a behaviour that would work for most instances. Maybe expose it in the clone itself and not as a new module?

clone.cloneInstance = cloneInstance;

return clone;

then clone(myInstance, clone.cloneInstance);

@millermedeiros please give your feedback about all of this

[millermedeiros]

I don't think we need object/cloneInstance it can be just lang/createObject (abusing the prototype chain) or the user can implement their own clone method on the Constructor.prototype. I think we are over-engineering.

As I said on the issue 1 lodash and underscore behavior differs (lodash works like mout). It seems that underscore followed prototype.js behavior (copies properties from the prototype as well) while we use forOwn for ALL methods so far:

$ npm install -g madge
$ madge -f amd -d object/forOwn src
  collection/forEach
  lang/clone
  lang/isEmpty
  object
  object/deepFillIn
  object/every
  object/fillIn
  object/filter
  object/keys
  object/map
  object/mixIn
  object/reduce
  object/size
  object/some
  object/values
  queryString/encode
$ madge -f amd -d object/forIn src
  object
  object/forOwn

A generic instanceClone could be implemented as:

function instanceClone(instance){
  return typeof instance.clone === 'function'? instance.clone() : createObject(instance);
}

But this would cause issues with jQuery since their $.fn.clone is actually to clone the matched elements not the jQuery instance itself.

We should also consider adding a shallowClone method or renaming clone into deepClone (probably better to rename to keep consistency).

The truth is, the only times I needed methods like lang/clone or object/mixIn I only cared about the object own properties and I've been using plain objects to store data way more often than custom types.

[millermedeiros]

uhm... just put more thought on it and I think custom types should not be cloned. There is no way to clone it automatically on a reliable way. Maybe we keep the 2nd argument in case user want to handle it but by default it should just return the instance itself without cloning.

// this is the behavior that I expect, custom types are not cloned, just passed by reference
// we only clone what we know we can clone safely
var a = { f: new Foo() };
var b = clone(a);
b.f === a.f; // true

[conradz]

@millermedeiros I think that would probably be the best way. I think it would be better to keep it simple, but allow the user to define the behavior if they need it.

[conradz]

I stripped the second commit, and defined the default behavior better now. It will now not attempt copying at all of instances created with a custom constructor, unless the cloneInstance function is provided.

[millermedeiros]

typo, it should be var g = clone(f, cloneCustom);

[conradz]

@millermedeiros Good catches, I fixed both of them now.

@satazor What do you think of this now? Would you agree with the way this works now?

[satazor]

Yes I agree. clone and deepClone sounds also good to me.

[millermedeiros]

feel free to merge, makes way more sense than current behavior.

PS: I will create a CHANGELOG.md file on the root of the repository now.