dojo.clone invokes functions

[Bryan Forbes]

Calling dojo.clone on an object with functions invokes those functions:

var someObj = {
    func: function(){
        console.log('someFunc.func');
    }
};
var someOtherObj = dojo.clone(someObj);

This will print 'someFunc.func' on the console.

[nic]

Proposed patch and a test page.
Maybe is there a better way to clone a function?

[Eugene Lazutkin]

The whole idea was that only objects were cloned. The rest (Booleans, numbers, strings, functions) were just copied. Anybody can look into the existing code and see it for themselves:

dojo.clone = function(/*anything*/ o){
  if(!o){
    // null, undefined, (and false, 0, "")
    return o;
  }
  if(dojo.isArray(o)){
    // special processing for arrays
  }
  if(!dojo.isObject(o)){
    // all non-objects are returned as is
    // FUNCTIONS SHOULD BE PROCESSED HERE
    return o;
  }
  if(o.nodeType && o.cloneNode){ // isNode
    // special processing for DOM nodes
  }
  if(o instanceof Date){
    // special processing for Date
    return new Date(o.getTime());
  }
  // Generic objects
  r = new o.constructor();
  // probably it should check if constructor is available
  // and revert to {}
  // ...
  return r; // Object
}

Somehow functions are not returned unchanged in some cases, but go down and processed as true objects.

We can clone functions like proposed but it is going to be much more complicated. Something like that:

// functions
r = function(){
  // notice that we lost performance right here
  // actually twice:
  // 1) an extra function wrapper
  // 2) apply() call, which is relatively expensive
  return o.apply(this, arguments);
}

// now let's copy all properties set on the original function
// including "prototype"
for(i in o){
  if(!(i in r) || r[i] != o[i]){
    r[i] = dojo.clone(o[i]);
  }
}
return r; // Object

I am not sure if it is worth it. But if functions are failed to be separated by !dojo.isObject(f), we have to include an extra check in any case (cloning, or treating as immutable values).

A side note for myself: change != to !== avoiding possible side-effects.

[James Burke]

dojo.isObject() uses dojo.isFunction() internally so we report Function as an object. Maybe changing the if(!dojo.isObject()) test to be something more like:

if(!o || typeof it != "object" || dojo.isFunction(o){

would be enough to fix the problem?

[nic]

if(!o || typeof o != "object" || dojo.isFunction(o){

Yes, it fixes the problem.

Is this ok too?

if(!dojo.isObject(o) || dojo.isFunction(o)){

[James Burke]

I was trying to avoid the extra function calls since isObject calls isArray and isFunction. The typeof one is about 8 characters longer, but I think it is OK since it is more explicit and faster.

[Eugene Lazutkin]

I fixed the problem as proposed in my local repository + added handling of "invisible" names (on IE) + expanded the unit test for dojo.clone(). But at the moment I cannot commit the change. I'll try to do it tomorrow.

[Eugene Lazutkin]

(In [19963]) dojo.clone() changes, !strict, fixes #9763.

[Adam Peller]

Apparently fixed on trunk only. I'd suggest we not backport it to 1.3 unless we can reduce what's currently a massive changeset and demonstrate a need and/or show that this is a recent regression. This sounds like #2456 which was around a long time.

[Justin Cooley]

This also happens when cloning a DOM node in IE. Testing o.cloneNode invokes cloneNode. Other browsers don't require a parameter, IE does and throws an error.

One solution:

// in dojo.clone
if(o.nodeType && "cloneNode" in o){ // isNode
  // special processing for DOM nodes
}

Another:

dojo.isDom = function(o){
   return(
      typeof Node == "object" ?
         o instanceof Node : 
         typeof o == "object" && typeof o.nodeType === "number" && typeof o.nodeName === "string"
   );
}

// in dojo.clone
if(dojo.isNode(o)){
   return o.cloneNode(true);
}

[Eugene Lazutkin]

(In [21196]) dojo.clone: changing the DOM node for IE, thx justinc!, !strict, fixes #9763.

[Eugene Lazutkin]

(In [21197]) dojo.clonedojo.clone: changing the DOM node for IE, thx justinc!, !strict, fixes #9763.