Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#9763 closed defect (fixed)

dojo.clone invokes functions

Reported by: Bryan Forbes Owned by: Eugene Lazutkin
Priority: high Milestone: 1.4.1
Component: Core Version: 1.3.2
Keywords: Cc: nicola.rizzo+dojo@…
Blocked By: Blocking:

Description

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.

Attachments (3)

test_clone.html (402 bytes) - added by Bryan Forbes 10 years ago.
lang.js.patch (471 bytes) - added by nic 10 years ago.
cloneTest.html (667 bytes) - added by nic 10 years ago.

Download all attachments as: .zip

Change History (16)

Changed 10 years ago by Bryan Forbes

Attachment: test_clone.html added

Changed 10 years ago by nic

Attachment: lang.js.patch added

Changed 10 years ago by nic

Attachment: cloneTest.html added

comment:1 Changed 10 years ago by nic

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

comment:2 Changed 10 years ago by nic

Cc: nicola.rizzo+dojo@… added

comment:3 Changed 10 years ago by Eugene Lazutkin

Status: newassigned

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.

comment:4 Changed 10 years ago by 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?

comment:5 Changed 10 years ago by 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)){

comment:6 Changed 10 years ago by 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.

comment:7 Changed 10 years ago by 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.

comment:8 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: assignedclosed

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

comment:9 Changed 10 years ago by Adam Peller

Milestone: 1.3.31.4

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.

comment:10 Changed 10 years ago by Justin Cooley

Resolution: fixed
Status: closedreopened

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);
}

comment:11 Changed 10 years ago by Eugene Lazutkin

Component: GeneralCore
Milestone: 1.41.4.1

comment:12 Changed 10 years ago by Eugene Lazutkin

Resolution: fixed
Status: reopenedclosed

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

comment:13 Changed 10 years ago by Eugene Lazutkin

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

Note: See TracTickets for help on using tickets.