Serialize: set empty string if value function returns null #3007
Conversation
|
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
|
@jtrumbull Thanks for the PR! This will need unit tests to prove that this PR fixes the linked issue. Tests for this module are located in https://github.com/jquery/jquery/blob/master/test/unit/serialize.js. |
|
@mgol Full test suite is passing. |
test/unit/serialize.js
Outdated
| params = { "test": [ 1, 2, null ] }; | ||
| assert.equal( jQuery.param( params, false ), "test%5B%5D=1&test%5B%5D=2&test%5B%5D=", "object with array property with null value" ); | ||
|
|
||
There was a problem hiding this comment.
Please revert the change in this line. We disallow trailing spaces.
There was a problem hiding this comment.
This applies to a few other changed lines as well.
|
@mgol I fixed the whitepace issue. However, this made me curious as to why an error wasn't being thrown by |
|
Ideally we'd check every file but those test files are very old, they contain lots of errors so fixing them is not an easy task... We're enabling JSCS file by file but we've got a long way ahead to lint everything. Source is linted completely, though, only tests are lacking. |
src/serialize.js
Outdated
| // If value is a function, invoke it and return its value | ||
| value = jQuery.isFunction( value ) ? value() : ( value == null ? "" : value ); | ||
| value = jQuery.isFunction( value ) ? value() : value; | ||
| value = value == null ? "" : value; |
There was a problem hiding this comment.
This may be smaller as if ( value == null ).
timmywil
changed the title
test/unit/serialize.js
Outdated
| assert.equal( jQuery.param( params, false ), "param1=", "Make sure that null params aren't traversed." ); | ||
|
|
||
| params = { "param1": function() {}, "param2": function() { return null; } }; | ||
| assert.equal( jQuery.param( params, false ), "param1=¶m2=", "object with method properties return null" ); |
There was a problem hiding this comment.
object with method properties return null -> Object with method properties that return null
I.e. start with a capital letter and fix one grammar issue in this sentence (as I understand it).
|
LGTM. |
5 participants
#3005