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 for multi-valued parameters #15

Closed
wants to merge 1 commit into from
Closed

Support for multi-valued parameters #15

wants to merge 1 commit into from

Conversation

cskr
Copy link

@cskr cskr commented Oct 31, 2010

HTTP allows having multiple values for the same parameter name. IncomingForm was not allowing this and always used the last value.

I've fixed this to use an array when multiple parameters are passed with same name and updated the test. Please pull.

@felixge
Copy link
Collaborator

felixge commented Oct 31, 2010

This is not a bug, the high-level interface (callback) is just a simplification. That being said, I would merge this feature as described here:

http://github.com/felixge/node-formidable/pull/10#issuecomment-489094

Your patch includes a little too much magic for my liking, as it would force users of the library to check each parameter if it is an array or not before accessing it.

@cskr
Copy link
Author

cskr commented Oct 31, 2010

Well since this is how node handles parameters (even in 0.3.0) I though it'd be fine. Anyway, the other approach looks fine too.

@felixge
Copy link
Collaborator

felixge commented Oct 31, 2010

How does node handle parameters?

@cskr
Copy link
Author

cskr commented Oct 31, 2010

querystring.parse('name=ABC&name=DEF') returns {name: ['ABC', 'DEF']}.

@felixge
Copy link
Collaborator

felixge commented Oct 31, 2010

Yeah, I don't think that's a good API. I mean it's cute and very tempting to implement it that way, but it can easily bring down a node app that does:

querystring.parse('name=ABC&name=DEF').name.substr(1);

I guess I should raise that as an issue with Ryan at some point.

@cskr
Copy link
Author

cskr commented Oct 31, 2010

But how else to allow parameters with multiple values? It is perfectly valid to have such parameters in HTTP.

@felixge
Copy link
Collaborator

felixge commented Oct 31, 2010

By either explicitly naming the parameters you want to treat as arrays or by enabling it for all. But just assuming that somebody wants both values and changing the underlaying datatype is very confusing. So it should be

querystring.parse('name=ABC&name=DEF', {arrayFields: true})

@coolaj86
Copy link

I thought the standard convention for array parameters was 'name[]=ABC&name[]=DEF'?

Isn't that the rule that every existing framework goes by at this point?
Think PHP (yuck), ruby, python, etc.

@cskr
Copy link
Author

cskr commented Jan 13, 2011

Suffixing [] is a convention used by frameworks in dynamic languages to decide whether to assign a value as a single item or as an array when there is only one parameter by that name. This is not required by HTTP and frameworks in statically typed languages don't use this convention.

As of node 0.2.x this convention of suffixing [] won't even work due the extra work node does to convert parameters to objects. It will work fine with 0.3.x though.

@cskr
Copy link
Author

cskr commented Jan 13, 2011

Aaargh! Clicked on "Comment and Close" instead of just "Comment". :(

This pull request was closed.
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.

None yet

3 participants