#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: | [email protected]… | |
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)
Change History (16)
Changed 12 years ago by
Attachment: | test_clone.html added |
---|
Changed 12 years ago by
Attachment: | lang.js.patch added |
---|
Changed 12 years ago by
Attachment: | cloneTest.html added |
---|
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
Cc: | [email protected]… added |
---|
comment:3 Changed 12 years ago by
Status: | new → assigned |
---|
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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
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 12 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:9 Changed 12 years ago by
Milestone: | 1.3.3 → 1.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 11 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 11 years ago by
Component: | General → Core |
---|---|
Milestone: | 1.4 → 1.4.1 |
comment:12 Changed 11 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Proposed patch and a test page.
Maybe is there a better way to clone a function?