Skip to content

Commit

Permalink
FBTest for Arr.isArrayLike()
Browse files Browse the repository at this point in the history
  • Loading branch information
@SebastianZ
SebastianZ committed Apr 11, 2014
1 parent 3cee896 commit cbfd684
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 0 deletions.
1 change: 1 addition & 0 deletions tests/content/firebug.html
View file
Expand Up @@ -36,6 +36,7 @@
*/
var testList = [
{group: "lib/array", uri: "lib/array/arrayInsert.js", desc: "Verify Arr.arrayInsert() is working as expected" },
{group: "lib/array", uri: "lib/array/isArrayLike.js", desc: "Verify Arr.isArrayLike() is working as expected" },
{group: "lib/array", uri: "lib/array/unique.js", desc: "Verify Arr.unique() is working as expected" },
{group: "lib/css", uri: "lib/css/classes.js", desc: "Verify Css.setClass(), Css.hasClass() and Css.removeClass() are working as expected" },
{group: "lib/search", uri: "lib/search/literalRegExp.js", desc: "Verify Search.LiteralRegExp() is working as expected" },
Expand Down
50 changes: 50 additions & 0 deletions tests/content/lib/array/isArrayLike.js
View file
@@ -0,0 +1,50 @@
function runTest()
{
function object(name)
{
this.name = name;
}

object.prototype.toString = function()
{
return this.name;
}

// Simulates a jQuery object
function jQuery()
{
this.splice = function()
{
console.log("splice me!");
}

this.length = 0;
}

var tasks = new FBTest.TaskList();
tasks.push(verifyResult, true, ["a"]);
tasks.push(verifyResult, true, document.getElementsByClassName("test"));
tasks.push(verifyResult, true, document.querySelectorAll("div"));
tasks.push(verifyResult, true, document.body.classList);
tasks.push(verifyResult, true, new jQuery());
tasks.push(verifyResult, false, "a");
tasks.push(verifyResult, false, 1);
tasks.push(verifyResult, false, null);
tasks.push(verifyResult, false, undefined);
tasks.push(verifyResult, false, NaN);
tasks.push(verifyResult, false, Infinity);
tasks.push(verifyResult, false, -Infinity);
tasks.push(verifyResult, false, new object("Peter"));
tasks.push(verifyResult, false, {hello: "Hello Firebug user!"});

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

Maybe add {0: "Hi", length: 1} -> false

This comment has been minimized.

Sign in to view

Copy link
@fflorent

fflorent Apr 11, 2014

Member

Are you sure it should return false?

>>> Array.forEach({0: "Hi", length: 1}, x => console.log(x));
Hi
undefined

Florent

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

I don't know. The question is more of less whether we should treat objects with a numeric length property as array-likes. But you also have to consider things like {width: 12, length: 10}. So I'm inclined to continue treating that as a non-arraylike.

And the forEach argument isn't very convincing; any object is a (0-sized) array by that measure. :)

>>> Array.forEach({hi: 'there'}, x => console.log(x));
undefined

This comment has been minimized.

Sign in to view

Copy link
@SebastianZ

SebastianZ Apr 11, 2014

Author Member

I added the test case in cb6dcb0.
I'll let you both argue about whether it should be considered as array or not. Though recognizing jQuery-like objects as arrays just because they contain a splice() function actually looks ugly to me and deserves a better approach.

Sebastian

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

Sure it's ugly, but it's somewhat of a standard and I don't think we should change it now. I think part of the reason jQuery added splice was (at least originally) just to be compatible with Firebug.

This comment has been minimized.

Sign in to view

Copy link
@SebastianZ

SebastianZ Apr 11, 2014

Author Member

but it's somewhat of a standard

To my knowledge it's not a standard that an object is considered as an array when it has a splice() method.

Sebastian

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

Well, "standard" in quotation marks. It's our previous behavior, and it's depended upon.

This comment has been minimized.

Sign in to view

Copy link
@SebastianZ

SebastianZ Apr 11, 2014

Author Member

and it's depended upon.

Where?

Sebastian

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

jQuery

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

There could be other places too, I don't know. Point is, I'd like to be conservative here, since what we have works so well.

This comment has been minimized.

Sign in to view

Copy link
@SebastianZ

SebastianZ Apr 11, 2014

Author Member

I have the feeling we're talking past each other. What I am saying is that we should find a better way to check for jQuery array-likes, not to give up support for them.

Sebastian

This comment has been minimized.

Sign in to view

Copy link
@simonlindholm

simonlindholm Apr 11, 2014

Member

I don't know. Wouldn't any such thing be as much of a hack as checking for "splice"? And there might be other libraries out there that have copied jQuery's hack. So yeah, I just don't see the point in finding another way, when this has already worked perfectly since forever.

This comment has been minimized.

Sign in to view

Copy link
@SebastianZ

SebastianZ Apr 11, 2014

Author Member

Wouldn't any such thing be as much of a hack as checking for "splice"?

Well, I don't know a better way to check it, otherwise I'd already have changed it.

And there might be other libraries out there that have copied jQuery's hack.

I still don't get why this should be a hack on jQuery side and not ours.

Anyway, it's just a nit, so IMO it's not worth to discuss that any further at this point.

Sebastian

tasks.run(FBTest.testDone, 0);
}

function verifyResult(callback, expected, variable)
{
var result = FW.FBL.isArrayLike(variable);
FBTest.compare(expected, result,
"Variable must" + (expected ? "" : " not") + " be an array-like object");

callback();
}

0 comments on commit cbfd684

Please sign in to comment.