figure out better specifications for this library

[jonathanong]

there are two issues blocking me from maintaining this library.

1. security issues
2. there's no consensus on how these nested query strings are supported

i'd like to have a consensus on how we should treat nested query strings before making any changes. in particular:

• should we even support objects nested more than X levels? i think nesting more than 2 levels is just silly. i don't even want to try supporting https://github.com/visionmedia/node-querystring/issues/30
• i'd like to throw when "arrays" do not consecutive indexes. i.e. a[1]= will throw. a[1]=&a[0] will throw. but a[0]=&a[1] will not throw. another option is just to convert those into objects and never arrays
• since we JS people use mongodb a lot, how about supporting mongodb-like syntax? a.0=&a.1=.
• should we just throw on weird query strings like https://github.com/visionmedia/node-querystring/issues/75 and https://github.com/visionmedia/node-querystring/issues/73? i don't see the point of even handling these cases
• do we want to optionally cast values? for example, might be nice to convert =true to true https://github.com/visionmedia/node-querystring/issues/34
• do we want to combine multiple values of the same apram like https://github.com/visionmedia/node-querystring/issues/38? i don't like how it creaets arrays - coudl be another option

basically, i want to be stricter and have options on the types of values returned. i also explicitly do not want to support params like https://github.com/visionmedia/node-querystring/issues/11.

[nlf]

Well, due to the stagnation of this library I ended up writing my own (see riddler) but I have some thoughts to share. It would be great if we could come to a consensus on some of these questions you have, since I shared the same struggles.

should we even support objects nested more than X levels? i think nesting more than 2 levels is just silly. i don't even want to try supporting #30

I made the nesting depth configurable, but defaulted to 5. I find it highly unlikely anyone will ever have to go deeper than that, but options are nice and it was trivial to add.

As for #30, the issue there isn't only the nesting but the formatting. As this module does, riddler bases object notation on square brackets [] not dots. Supporting both is possible, but I'm not sure about the merits. It seems like people have been content with qs not supporting dot notation, so is it really that important?

i'd like to throw when "arrays" do not consecutive indexes. i.e. a[1]= will throw. a[1]=&a[0] will throw. but a[0]=&a[1] will not throw. another option is just to convert those into objects and never arrays

I don't throw errors in these situations. I create a sparse array with the values in the requested indices, then compact it to only the values that exist while preserving order. This seemed like the fair compromise to me.

since we JS people use mongodb a lot, how about supporting mongodb-like syntax? a.0=&a.1=.

This is the same question as what I was mentioning above. Should it support only dots? Only square brackets? Both? I don't think supporting both would be too terribly difficult, but it's hard to say without getting started and finding out.

should we just throw on weird query strings like #75 and #73? i don't see the point of even handling these cases

I prefer not to throw with invalid strings, but return what you are able to parse and remove the rest. I think the current behavior in #73 is correct. As for #75 I think a return value of { y: '1' } seems reasonable, as we simply drop the invalid portion of the string.

That's just me though, I'd love to hear more thoughts about throwing errors instead.

do we want to optionally cast values? for example, might be nice to convert =true to true #34

Again, this is another one I debated in my head. Casting values isn't overly difficult, and could be a positive gain, but I'm not sure how worthwhile it is. I may attempt it and see how much additional complexity it adds.

do we want to combine multiple values of the same apram like #38? i don't like how it creaets arrays - coudl be another option

My hunch here is actually to change things and take the last item instead of returning an array, as that seems to be the consensus amongst other frameworks.

Regarding #11, I think bailing on nesting and returning { '[foo]': 'bar' } is the appropriate behavior.

[sebs]

I see no problem with a fork and I guess it is a good thing what you did. This all sounds pretty good to me when I flash over the post. Why dont you and TJ get on a skype call phone and talk directly about this. Not that I dont like the ideas of forks, but your work could re-animate this project

[nlf]

I'm having second thoughts about my stance regarding #38 and I think I'm going to keep the current behavior of merging into an array, unless someone can present a very strong reason not to..

[nlf]

So while attempting to fix both #11 and #75, I stumbled across this behavior:

riddler.parse('[]&y=1');
// { '0': '', y: '1' }

riddler.parse('[foo]=bar');
// { foo: 'bar' }

For my code base, I did this by just assuming if no prefix exists that I should continue parsing regardless.

[] gets converted to the array [ '' ], and when merged with the original base object {} it gets coerced to an object and the result is { '0': '' }.

This also means that riddler.parse('[]=a&[]=b') would yield the object { '0': 'a', '1': 'b' }.

I'm considering changing this so that if only an array exists at the root, as is the case with []=a&[]=b that I return an actual array (in that example ['a', 'b']), but I'm mostly content with this resolution.

[jonathanong]

I don't throw errors in these situations. I create a sparse array with the values in the requested indices, then compact it to only the values that exist while preserving order. This seemed like the fair compromise to me.

i think this is a security issue, i'm not 100% sure though.

I prefer not to throw with invalid strings, but return what you are able to parse and remove the rest.

might confuse some people. maybe returning a separate object of "WTF"s is better.

[jonathanong]

i don't think TJ cares about this module anymore. none of the "previous" maintainers of express care much either (including myself). this library is really more trouble than its worth. might be easier just to JSON in the query string fields. hahaha

i also don't think a fork is necessary. @nlf if you'd like to maintain this project and merge in your riddler ideas, i'm sure TJ wouldn't mind adding you as collab

[tj]

eek yeah so many edge-cases haha, I've tried my best to avoid super weird requests that require anything that a browser form submission doesn't create. fork and/or contrib doesn't matter to me!

the parse array thing works fine until someone iterates over it via console.log / json.stringify or similar

[nlf]

It was a security issue, but I actually tweaked things so that the specific index depth is limited. If you attempt to assign to an array index over the limit, you get an object with that number as the key instead.

As far as forking, I've already completely reimplemented the entire library in riddler. I didn't base it off of this module though, so merging it back in isn't really an option. It would be more like replacing everything.

My thought is to either deprecate this library entirely and refer people to riddler, or publish a copy of riddler in its place. Thoughts?

[tj]

haven't really looked at riddler to know what the differences are but either sounds fine to me assuming riddler supports the same things, not the worst thing to have two either I'm sure there are many more in npm

[nlf]

Yeah, riddler supports all the same things.

My personal feeling, especially if this is going to stay unmaintained, is that the best course of action is to deprecate this library and refer people to riddler.

[jonathanong]

i don't mind replacing this, or just have riddler be qs v1+ :)

[tj]

SGTM

[jonathanong]

dev has moved to https://github.com/hapijs/qs :D

[kenperkins]

I'd argue that dev has not "moved" to hapijs/qs. Rather, hapijs/qs is just a new repository. There's no history, etc. Why was this not moved via github?

[dougwilson]

Why was this not moved via github?

visionmedia historically does not move repos. Check out should.js as another example: https://github.com/visionmedia/should.js/#repo-was-moved-to-own-organization-see-httpsgithubcomshouldjsshouldjs

[kenperkins]

Just because that's what's been done doesn't mean it's a good decision. Especially lacking insight as to why, I find this really confusing.

[dougwilson]

IDK what you want us to say :) Ask @visionmedia :)

[tj]

it's a rewrite, doesn't really matter, move would be fine. As for should.js it doesn't make any sense to me, it's a single project, why move it to an org?

[kenperkins]

You move because then all existing installs that point to the old repo automatically get a 302 to the new location. Issues, commits, etc all persist.

[kenperkins]

And no weird "now deprecated" messages as well.