Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support objects with numeric keys. #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abecciu
Copy link
Contributor

@abecciu abecciu commented May 3, 2013

No description provided.

@abecciu
Copy link
Contributor Author

abecciu commented May 21, 2013

@visionmedia I'd really like to get this merged. What do you think?

Tests are failing in master too for node 0.11.

@tj
Copy link
Owner

tj commented May 22, 2013

this case already works:

> require('./').parse('123=foo')
{ '123': 'foo' }

as for the quoted ones I'm not a huge fan of that personally, what's the use-case?

@abecciu
Copy link
Contributor Author

abecciu commented May 22, 2013

@visionmedia I have a particular use-case where I need to have hashes with numeric keys stored in a parent hash.

Here's an example with qs master:

> qs.parse(qs.stringify({a: { '123': { a: 2 } } }))
{ a: [ { a: '2' } ] }

Rather than creating a hash of numeric keys, it treats the numbers as indexes of an array.

My solution is to escape the numeric keys in an object with single quotes, so that the parser is not confused with array indexes.

This is my version:

> qs.parse(qs.stringify({a: { '123': { a: 2 } } }))
{ a: { '123': { a: '2' } } }

@abecciu
Copy link
Contributor Author

abecciu commented May 29, 2013

@visionmedia sorry for keep pushing, but I really want to get this merged. I think it's an edge case that should be handled correctly and of course it's super useful to me. :)

Anything else I should do?

Thanks!

@tj
Copy link
Owner

tj commented May 31, 2013

needs to support double quotes as well if we're going that route

@abecciu
Copy link
Contributor Author

abecciu commented Jun 1, 2013

@visionmedia just added support for double quotes. Thanks!

@kamikat
Copy link

kamikat commented Jul 4, 2013

Have the same issue with qs parsing foo[1002]=bar&foo[9039]=baz which the number 1002 and 9039 are actually strings in application and expected be parsed to {foo: {'1002': 'bar', '9039': 'baz'} }.

@abecciu
Copy link
Contributor Author

abecciu commented Jul 4, 2013

@shinohane this patch fixes that.

@MitchellMcKenna
Copy link

Related Issue: #14

@buschtoens
Copy link
Collaborator

Can you add an example, what's not working?

@silkcom
Copy link

silkcom commented Mar 16, 2014

I fail to see why this wouldn't just allow without quotes at all, and just treat them as objects if indices are set. Example:

qs.parse(qs.stringify({a: { 123: { a: 2 } } }))
{ a: { 123: { a: 2 } } }

or

qs.parse(qs.stringify({a: { '123': { a: 2 } } }))
{ a: { '123': { a: 2 } } }

@silkcom
Copy link

silkcom commented Mar 18, 2014

What would it take to get this merged in? BodyParser is based off this, and it's broken for cases such as having items[1] = on, and item[2] = on. Should be { items: {1: "on", 2: "on"} } but it's { items: [on, on] } instead. Which makes it completely worthless.

@silkcom
Copy link

silkcom commented Mar 19, 2014

looking over the failure here, it seems like something in node .11 removes hasOwnProperty which seems like it's a problem not related with this fix (though it's possible this fix is showing off the problem.

@silkcom
Copy link

silkcom commented Mar 19, 2014

nm, looking it over, master needs to be merged into this. Master has an extra hasOwnProperty call. I really think though that the quotes should be optional. The isint check should just be removed entirely and if there is something inside the []'s it's an object, if there's not it's an array

@silkcom
Copy link

silkcom commented Mar 20, 2014

created #98 which doesn't fix the quotes, but it does make {}'s the default

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants