Skip to content
Permalink
Browse files
Fix #10466. jQuery.param() should treat object-wrapped primitives as … 
…primitives.
  • Loading branch information
@rwaldron @dmethvin
rwaldron authored and dmethvin committed Dec 6, 2011
1 parent 6c2a501 commit 166b9d2
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
2 src/ajax.js
View file
@@ -818,7 +818,7 @@ function buildParams( prefix, obj, traditional, add ) {
}
});

} else if ( !traditional && obj != null && typeof obj === "object" ) {
} else if ( !traditional && jQuery.isPlainObject( obj ) ) {
// Serialize object item.
for ( var name in obj ) {
buildParams( prefix + "[" + name + "]", obj[ name ], traditional, add );
13 test/unit/ajax.js
View file
@@ -1031,6 +1031,19 @@ test("jQuery.param()", function() {
equal( jQuery.param( params, false ), "test%5Blength%5D=3&test%5Bfoo%5D=bar", "Sub-object with a length property" );
});

test("jQuery.param() Constructed prop values", function() {
expect(3);

var params = {"test": new String("foo") };
equal( jQuery.param( params, false ), "test=foo", "Do not mistake new String() for a plain object" );

params = {"test": new Number(5) };
equal( jQuery.param( params, false ), "test=5", "Do not mistake new Number() for a plain object" );

params = {"test": new Date() };
ok( jQuery.param( params, false ), "(Non empty string returned) Do not mistake new Date() for a plain object" );
});

test("synchronous request", function() {
expect(1);
ok( /^{ "data"/.test( jQuery.ajax({url: url("data/json_obj.js"), dataType: "text", async: false}).responseText ), "check returned text" );

11 comments on commit 166b9d2

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 166b9d2 Dec 8, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jQuery.isPlainObject is unnecessary here - use the faster jQuery.type( obj ) === "object".

Btw.:
Few lines up, there is typeof v === "object" || jQuery.isArray(v) that can be simplified to typeof v === "object".

Also, if really wonted, the isPlainObject can be avoided in .param too, by replacing a.jquery && !jQuery.isPlainObject( a ) with a.jquery && !hasOwn.call( a, "jquery" ), but this would require hasOwn = Object.prototype.toString on the top - so probably not too beneficial considering that isPlainObject is called only once here.

If you want I can make a pull req.

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rkatic, I doubt this would yield any measurable performance improvement in real code, given how seldom this is called. Would you be interested in taking on a larger project for 1.8? These micro-optimizations often are more bother to change than they are worth, we need to be focusing on big gains.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 166b9d2 Dec 8, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, the last one is certainly an over-optimization, but I pointed it now in case something like that will be needed...
Other two, however, was more for correctness...

Would you be interested in taking on a larger project for 1.8?

Do you have something in particular in mind?
I have intention to try to contribute to jQuery in next months anyway.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous comment's test was backwards, strike that.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, jQuery.type(foo) will definitely work fine, but I'm not sure it's worth it? http://jsperf.com/type-vs-isplainobject

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After making the change, byte counter reports:

jQuery Size - compared to last make
  248549     (+4) jquery.js
   93704     (+2) jquery.min.js
   33275      (-) jquery.min.js.gz

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 166b9d2 Dec 8, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more about correctness. Consider:

function Record(name, id) {
  this.id = id;
  this.name = name;
}

$.param({"boo" : new Record("foo", 666) });

Why to make this unnecessarily invalid?

Also, an more honest perf would probably be http://jsperf.com/type-vs-isplainobject/2.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is that a "more honest perf"??? Because you said so? All you've done is create more noise.

This is the BEST argument given:

It's more about correctness. Consider:

function Record(name, id) {
  this.id = id;
  this.name = name;
}


$.param({"boo" : new Record("foo", 666) });

Why to make this unnecessarily invalid?

Why didn't you lead with that argument instead of nitpicking some half-cocked performance micro-optimization?

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 166b9d2 Dec 8, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the first time that we do not agree on writing perfs, even if I am simply taking in consideration different types... but at the end the ideal perf is probably something in the middle: making more probable types more frequent, but not ignoring others too.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to account for other types, do so in separate tests - it hardly makes sense to cram 3 different operations into each test.

@rkatic
Copy link
Contributor

@rkatic rkatic commented on 166b9d2 Dec 8, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that would be even better, but I am not in the mood to make N tests for something like this...

Please sign in to comment.