change lang/clone behavior to copy all enumerable properties and add flag to clone instances

[millermedeiros]

use object/forIn instead of object/forOwn to clone object properties.

add a flag to toggle behavior that way properties from the prototype will be copied by default and user will also have the option to use the prototype chain to clone instances (objects created using custom constructors).

waiting for feedback before merging

see #116

[conradz]

I currently don't see any good reason to copy prototype properties. I know underscore.js does this, but it seems that copying prototype properties could make it easier for bad programming (e.g. cloning a jQuery object probably would work, but would be extremely bad programming). I can't currently think of any good use for cloning prototype properties, since usually if you need to clone some object with a prototype there is usually an existing way for cloning it built-in to the library (e.g. use jQuery's .clone or create a new jQuery object).

[millermedeiros]

@conradz let's say you have some config object that is created by inheriting some properties from the prototype (eg: new UserSettings()) - I rarely write code like this but it might be useful to other users. Since lodash and underscore follows this behavior I guess it's better to follow it as well (principle of least astonishment). I agree that it shouldn't be the preferred method for cloning non-native objects (maybe we should add a note to the documentation).

I just realized that I should probably check if the object was really created by a custom constructor before using createObject if shouldCloneInstances is true.

[conradz]

OK, never thought about inherited config properties. I guess that could be a valid use case for this behavior. I would agree on adding a note to the docs.

As a side note, obj.constructor could be faked. AFAIK checking .constructor is the best we can do for detecting a custom type. The following code is an example of how clone could return an object with different methods from the original if it uses the .constructor property:

function MyType() {}
MyType.prototype.foo = function() { console.log("foo"); };
var bad = { constructor: MyType };
var cloned = clone(bad);

cloned.foo === bad.foo // This will be false, since the cloned object will have the prototype, and the original actually did not

If this is behind a flag, it shouldn't pose much problem, because anyone using the flag should know what they are doing, and should not arbitrarily set the constructor property.

Although with the current (I assume wrong according to your previous comment) behavior, all cloned objects are not actually cloned, just new objects with the old object as there prototype. See this line.

[satazor]

I just realized that I should probably check if the object was really created by a custom constructor before using createObject if shouldCloneInstances is true.

Yep.

If this is behind a flag, it shouldn't pose much problem, because anyone using the flag should know what they are doing, and should not arbitrarily set the constructor property.

Agree. I think it's not a problem, because the flag is meant to be used in advanced cases. Whoever is setting it to true knows clearly what's being done.