-
Notifications
You must be signed in to change notification settings - Fork 66
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
Handle partially parsed objects (w/ querystrings as keys) #27
base: master
Are you sure you want to change the base?
Conversation
- previously this would work in some cases, this fix handles most cases properly - { 'foo[bar]': { qux: 0 }, 'foo': { baz: 1 } } => { foo: { bar: { qux: 0 }, baz: 1 } } - allows for handling of pre-parsed multipart forms from felixge/node-formidable
@@ -232,10 +224,13 @@ function stringifyObject(obj, prefix) { | |||
|
|||
function set(obj, key, val) { | |||
var v = obj[key]; | |||
if (undefined === v) { | |||
if ('undefined' == typeof v) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this? where's v coming from that we need typeof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh it's right above haha, we dont need typeof then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just consistency really, and because undefined
can be redefined in Javascript, meaning the safest way to check for undefined
is with typeof
. It's a habit of mine...
EDIT: I knew this, but just tried it... blows my mind:
> typeof undefined
'undefined'
> {}['foo'] === undefined
true
> undefined = 'foo';
'foo'
> typeof undefined
'string'
> {}['foo'] === undefined
false
EDIT 2: Apparently this behavior was eliminated in newer versions of V8? The above was in Node v0.4.8, v0.6.2 doesn't allow it... thank goodness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah gotcha, personally I avoid defensive programming whenever possible to delegate the issue. plus there are a ton of retarded things you can do in js, people just shouldn't do them haha
Ok, so I've found a weird edge case that I'd like to get your feedback on before you do anything with this: Current behavior: > console.log(qs.parse({ 'foo[items]': [], foo: { items: ['bar'] } }))
{ "foo": { "items": [ [ "bar" ] ] } }
> console.log(qs.parse({ foo: { items: ['bar'] }, 'foo[items]': [] }))
{ "foo": { "items": [ "bar", [] ] } } Oddly enough, we actually ran into this situation with RestKit (starting to hate it). We were expecting a consistent output, regardless of order, but unfortunately there isn't one when you use the "rules" of querystrings: that repeated keys are pushed together into arrays. The output above actually makes sense given the rules—its just not what we expected or wanted. One possible solution would be to use Using concat() instead of push(): > console.log(qs.parse({ 'foo[items]': [], foo: { items: ['bar'] } }))
{ "foo": { "items": [ "bar" ] } }
> console.log(qs.parse({ foo: { items: ['bar'] }, 'foo[items]': [] }))
{ "foo": { "items": [ "bar" ] } } To be honest, I don't really like either option—this whole thing seems really weird. Thoughts? |
my vote is on the second I think |
tl;dr
Why?
For multipart forms, particularly from node-formidable. This particular use case is important:
We can use mimetypes to partially parse data from node-formidable parts, before using qs to reassemble
Does we really need this?
Yes. The RestKit iOS framework (among others) can send multipart forms containing both
application/json
and attachment (ie.image/jpeg
) data. Properly merging the parts back into a single params object is crucial. The ExpressJS body-parser got us most of the way there—although we did have to overridehandlePart()
so we could parse the JSON based on the mimetype.The final piece of the puzzle is getting qs to reassemble the parts from the querystring paths as expected. As I mentioned in the commit message, qs was doing this properly in some cases, but not all (depending on part order from node-formidable, which is apparently non-deterministic). This fix should handle most cases as expected.