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

Use query-string instead of qs #171

Merged
merged 1 commit into from
Dec 6, 2015
Merged

Use query-string instead of qs #171

merged 1 commit into from
Dec 6, 2015

Conversation

mjackson
Copy link
Member

@mjackson mjackson commented Dec 6, 2015

Fixes #121


function isNestedObject(object) {
for (var p in object)
if (typeof object[p] === 'object')
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you want to handle arrays and null here? query-string handles them just fine (though stringified differently from qs in both cases).

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it handles those sanely. This is just a developer convenience warning, so they're aware we're not looking to handle nested queries. I wouldn't try to go overboard on supporting a bunch of edge cases here. I think they'll figure it out when they see %5Bobject+Object%5D in their URLs 😄

@timdorr
Copy link
Member

timdorr commented Dec 6, 2015

@mjackson Feel free to smash that commit. I don't know how you like your commit history. I'm more of a multiple-commit kind of dude, but you may want it clean.

@taion
Copy link
Contributor

taion commented Dec 6, 2015

@timdorr Regarding your line comment that got eaten, what I meant was we should probably not warn in those cases, because query-string is sane about handling them.

@timdorr
Copy link
Member

timdorr commented Dec 6, 2015

Duh, because arrays are typeof object. I always forget that. Let me amend that commit then.

@timdorr timdorr force-pushed the query-string branch 2 times, most recently from f392434 to 9277c51 Compare December 6, 2015 15:54
@taion
Copy link
Contributor

taion commented Dec 6, 2015

Arrays and null. I usually just do p instanceof Object.

@timdorr
Copy link
Member

timdorr commented Dec 6, 2015

But [] instanceof Object === true

@taion
Copy link
Contributor

taion commented Dec 6, 2015

D'oh. Let's squash and merge this then.

timdorr added a commit that referenced this pull request Dec 6, 2015
Use query-string instead of qs
@timdorr timdorr merged commit 0a1b31a into master Dec 6, 2015
@timdorr timdorr deleted the query-string branch December 6, 2015 16:07
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants