PDA

View Full Version : [FIXED-88] [3.x] Ext.isObject([]) returns true



mystix
7 Jul 2009, 11:39 PM
Ext.isObject([]) currently returns true.

the following override resolves this issue:


Ext.apply(Ext, {
isObject: function(v) {
return v && Object.prototype.toString.call(v) == '[object Object]';
}
});


the check in Ext.data.Record also needs to use this new Ext.isObject() method:


Ext.override(Ext.data.Record, {
set: function(name, value) {
var encode = Ext.isPrimitive(v) ? String : Ext.encode;
if (encode(this.data[name]) == encode(value)) {
return;
}
this.dirty = true;
if (!this.modified) {
this.modified = {};
}
if (this.modified[name] === undefined) {
this.modified[name] = this.data[name];
}
this.data[name] = value;
if (!this.editing) {
this.afterEdit();
}
}
});

(note: also optimised the before / after value comparison above)

Condor
7 Jul 2009, 11:46 PM
I agree that isObject should not return true for arrays, but there are several places in the Ext code that use the isObject test, but should also execute for arrays.

Some of these isObject checks need to be replaced by !isPrimitive or !isString tests.

Your own code is a good example of that:

var encode = Ext.isPrimitive(v) ? String : Ext.encode;

evant
7 Jul 2009, 11:55 PM
The sample applies for a number of cases:



Ext.onReady(function(){
console.log(Ext.isObject([]));
console.log(Ext.isObject(document.getElementsByTagName('*')));
var f = function(){
console.log(Ext.isObject(arguments));
};
f();
});


In all of these cases, technically they ~are~ objects.

Condor
8 Jul 2009, 12:02 AM
In all of these cases, technically they ~are~ objects.

If you use that logic then you need to go through all Ext.isObject calls in the Ext code and check if they also should execute for arrays (most of them shouldn't!).

And don't forget that typeof new Date() == 'object'.

evant
8 Jul 2009, 12:05 AM
True, I believe the intent for the method is to exclude "iterable" types.

mystix
8 Jul 2009, 12:07 AM
yes, an array is technically a js Object, but isn't the Ext.isObject() method supposed to be able to distinguish between "real" js objects vs js arrays?

[edit]
you guys type too fast *LOL*

mystix
8 Jul 2009, 12:10 AM
I agree that isObject should not return true for arrays, but there are several places in the Ext code that use the isObject test, but should also execute for arrays.

Some of these isObject checks need to be replaced by !isPrimitive or !isString tests.

Your own code is a good example of that:

var encode = Ext.isPrimitive(v) ? String : Ext.encode;

good point -- i simply refactored for compactness without considering that.

[edit]
updated my initial post to match

mystix
8 Jul 2009, 12:16 AM
i guess the form which filters out Dates / Arrays etc would be


Ext.apply(Ext, {
isObject: function(v) {
return v && Object.prototype.toString.call(v) == '[object Object]';
}
});


[edit]
updated my initial post to match

mystix
20 Jul 2009, 2:42 AM
[ friendly bump ]

Tom23
20 Oct 2009, 5:59 AM
Ext.isObject([]) currently returns true.

the following override resolves this issue:


Ext.apply(Ext, {
isObject: function(v) {
return v && Object.prototype.toString.call(v) == '[object Object]';
}
});


Plus, Ext.isObject(undefined) returns undefined, Ext.isObject(0) returns 0, Ext.isObject('') returns '', and Ext.isObject(null) returns null.

Should be

Ext.apply(Ext, {
isObject: function(v) {
return !!v && Object.prototype.toString.call(v) == '[object Object]';
}
});Edit: I think if Object.prototype.toString is used, the "... &&" part is not even needed (?)

TommyMaintz
29 Oct 2009, 9:38 AM
I have scanned through the Ext Core code and some of the Ext JS code and so far haven't been able to find places where we expect Ext.isObject to return true for an array. Condor, do you have any examples of places where we do so?

There are 4 different options as far as I can think of:



var objReg = /^\[object (Object|HTML|XML)/;
isObject1 = function(o) {
return objReg.test(Object.prototype.toString.call(o));
}

isObject2 = function(o) {
return !!o && typeof o === 'object';
}

isObject3 = function(o) {
return o.constructor === Object;
}

isObject4 = function(o) {
return Object.prototype.toString.call(o) === '[object Object]';
}


Now this will give the following results



SomeClass = function() {
this.someproperty = 'test';
}
SomeClass.prototype = {
someMethod: function() {}
}

// true isObject1({key: 'value'})
// true isObject1(document.createElement('div'))
// true isObject1(new SomeClass())
// true isObject1(new XMLHttpRequest())
// false isObject1([])
// false isObject1('string');

// true isObject2({key: 'value'})
// true isObject2(document.createElement('div'))
// true isObject2(new SomeClass())
// true isObject2(new XMLHttpRequest())
// false isObject2([])
// false isObject2('string')

// true isObject3({key: 'value'})
// false isObject3(document.createElement('div'))
// true isObject3(new SomeClass())
// false isObject3(new XMLHttpRequest())
// false isObject3([])
// false isObject3('string')

// true isObject4({key: 'value'})
// false isObject4(document.createElement('div'))
// true isObject4(new SomeClass())
// false isObject4(new XMLHttpRequest())
// false isObject4([])
// false isObject4('string')


I think both isObject 3 and 4 are the most correct, but they might break existing code. isObject 1 is more flexible in that it will be able to accept HTML elements, XMLHttpRequest, and key/value pairs, but not arrays etc.

isObject 2 is what we currently have and will accept just about anything as an object, which is wrong imo.

So which one of those 4 do you guys think is the most "correct"?

Tom23
29 Oct 2009, 10:11 AM
isObject3 may fail in cross-frame operations, because different frames have different Object objects (confusing, eh?)


document.body.appendChild(document.createElement('iframe'));
xObject = window.frames[window.frames.length-1].Object;
var obj = new xObject();
obj instanceof Object; // false
obj instanceof xObject; // true
credits: Douglas Crockford (http://groups.google.com/group/comp.lang.javascript/msg/1afbcb0da1cd4aef)

TommyMaintz
29 Oct 2009, 10:41 AM
Thanks Tom, I had already read the same thing ;)

I chose to go for option four. I have also added a function Ext.isElement which returns !!v && v.tagName. Lastly I searched through the whole ExtJS library for wrong uses of isObject. I have found 3, but if someone thinks I have missed something, please let me know.

Fixed in revision #5564