_.toQuery fix

[jgonggrijp]

This branch adresses #217. For a query payload like {a: [], b: []}, I had two options to choose from: 'a=&b=' or just the empty string ''. Since {a: []} already encoded to the empty string, the empty string also seemed to be the right generalization for multiple empty arrays. This choice is further justified by two other facts: _.fromQuery('a=') returns {a: ''} rather than {a: []}, and jQuery encodes empty array parameters as empty strings, too.

@mafiosso @sktguha @yashshah1 @carpben if one of you could review this, that would be great.

[sktguha]

ok will try to review tomorrow as its late night right now

[carpben]

deleted

[carpben]

@jgonggrijp, I think your reasoning for converting x:[] to an empty string is solid. I do have a few comments in regards to the implementation.

1. This will convert any x:falsy key-value pair to an empty string. Currently x:null is converted to x=null& (same for undefined). Returning an empty string instead might be worthy of consideration, but might actually be a breaking change. x=null might be a way to signal that the value of x isn’t the default or expected or the previous value. Might be better to introduce such a change in a major version.
2. The same policy should be applied to both objects and arrays. According to this PR a falsy value in an object, and a falsy value in an array are treated differently.

I suggest we solve it from a different angle. As mentioned in #217
_.toQuery({a:[], b:[], c:[]}) is converted to "&&".

The problem is that empty string elements are joined with an & in between. I suggest we solve it by filtering empty strings.
I'll be happy to make a commit on top of your PR.

[yashshah1]

@jgonggrijp Other than the issue caused by null, undefined or other falsy objects, this seems to be perfect.

A side note: JQuery.param() replaces occurrences of null and undefined with '', so that seems like acceptable behavior. Pen

[jgonggrijp]

Thanks @carpben and @yashshah1 for both taking the effort to review!

1. This will convert any x:falsy key-value pair to an empty string. Currently x:null is converted to x=null& (same for undefined). Returning an empty string instead might be worthy of consideration, but might actually be a breaking change. x=null might be a way to signal that the value of x isn’t the default or expected or the previous value. Might be better to introduce such a change in a major version.

You are right to be concerned about this, but fortunately, there is no such change. {a: null} is still encoded as a=null and likewise for undefined. _.compact is applied post-recursion, so it only filters out already encoded substrings that happen to be empty. I have, however, added a test to verify this.

2. The same policy should be applied to both objects and arrays. According to this PR a falsy value in an object, and a falsy value in an array are treated differently.

Good catch! I added a _.compact around the array recursion as well.

I suggest we solve it from a different angle. As mentioned in #217
_.toQuery({a:[], b:[], c:[]}) is converted to "&&".

The problem is that empty string elements are joined with an & in between. I suggest we solve it by filtering empty strings.

If I'm right, this is what I did. Still reassuring that you arrive at the same conclusion independently, though.

I'll be happy to make a commit on top of your PR.

Thank you for the kind offer. If I'm right, it isn't necessary, but please do push a commit if you still think that I'm overlooking something.

[yashshah1]

@jgonggrijp had another question; while encoding null/undefined as strings, does fromQuery take care of parsing them as null/undefined or will it become the string representation?

[jgonggrijp]

@yashshah1 Good question. It appears that _.fromQuery is entirely "stringly typed", probably due to query string syntax not being expressive enough to support other data types. I just ran this experiment:

_.fromQuery('a=&b=null&c=undefined&d=10')
// { a: '', b: 'null', c: 'undefined', d: '10' }

[yashshah1]

The number / string would make sense, as it's upto the the receiver to parse all numeric types.

However, I feel that null and undefined should be marked seperately.

One approach could be assigning null strings to be empty strings (which quite appropriately signifies falsy-ness), and a breaking change to not include any undefined values in the output.

So any keys with undefined values could be ignored from the query string.

[carpben]

I was obviously wrong in my first comment. _.compact in your PR doesn't have to deal with null but only with strings. My suggestions that follow this wrong assumption are irrelevant. Great job @jgonggrijp !

[jgonggrijp]

@yashshah1

The number / string would make sense, as it's upto the the receiver to parse all numeric types.

However, I feel that null and undefined should be marked seperately.

One approach could be assigning null strings to be empty strings (which quite appropriately signifies falsy-ness), and a breaking change to not include any undefined values in the output.

So any keys with undefined values could be ignored from the query string.

If I understand correctly, you're suggesting changes to the _.fromQuery function. If I'm right, please create a new issue ticket. The current PR is only about fixing a bug in _.toQuery.

@carpben

Great job @jgonggrijp !

Thanks!

[yashshah1]

@jgonggrijp
I apologise for the vague wording in the previous comment. What I am suggesting is a change in both toQuery and fromQuery.

Proposed behaviour:

_.toQuery('{ a: '', b: null, c: undefined, d: '10' }')
// a=&b=&d=10

_.fromQuery('a=&b=&d=10')
// { a: '', b: '', d: '10' }

Reasoning
The idea is that toQuery and fromQuery need to be as complementary to each other as possible, one drawback that's seen already is that numbers need to be re-parsed. Wrt falsy values, however, I think there has to be a special provision as described above.

What we gain by this is that null values are re-encoded as empty strings, which is falsy, and undefined isn't included in the object, which leads to almost the same usage in a few cases.

var obj = { a: '', b: null, c: undefined, d: '10' }
console.log(obj['c']) // undefined

obj = _.fromQuery(_.toQuery(obj))
console.log(obj['c']) // undefined

This might lead to a change in the behaviour of fromQuery and I am happy to raise a PR should this be acceptable.

[jgonggrijp]

@yashshah1 Thanks for elaborating and for thinking along. It is much clearer to me now what you're after.

You may have a point, but I still think it doesn't belong in this PR. I'd also prefer that you open a new issue ticket, rather a PR, because I'd like to move towards stage 4 of #220 as soon as possible. You can reuse the text from your last post in the new issue. Note that you can edit your post in order to get the exact markdown source code.

I think we're done with this particular bugfix, so I'm merging the branch. Again, thanks everyone for your involvement!