fix: Add brackets to querystring to clarify it's array (#1672)

[3jins]

If length of array is 1, it was being recognized as a string, not an array.

• AS-IS:
  { colors: ['black', 'mustard'] } -> colors=black&colors=mustard
  { colors: ['black'] } -> colors=black
• TO-BE:
  { colors: ['black', 'mustard'] } -> colors[]=black&colors[]=mustard
  { colors: ['black'] } -> colors[]=black

Added qs dependency for this fix.

Checklist

• I have ensured my pull request is not behind the main or master branch of the original repository.
• I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
• I have written a commit message that passes commitlint linting.
• I have ensured that my code changes pass linting tests.
• I have ensured that my code changes pass unit tests.
• I have described my pull request and the reasons for code changes along with context if necessary.

[codecov]

Codecov Report

Merging #1673 (4a53007) into master (00ce71f) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1673   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files           5        5           
  Lines         507      507           
  Branches      142      142           
=======================================
  Hits          505      505           
  Misses          2        2           

Impacted Files	Coverage Δ
lib/request.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00ce71f...4a53007. Read the comment docs.

[miwnwski]

I'm for this change, however I fear this can break a lot of applications because users might rely on current behaviour today. I wouldn't be comfortable releasing this under Koa 2.x

[miwnwski]

LGTM

Seems like a decent change but I'm going to let other maintainers decide on this one.

[3jins]

Is anyone willing to merge this PR?
I don't have write access so that I cannot merge this.

[3imed-jaberi]

@miwnwski From my side, in theory yes we can merge this PR now since we are on v3-alpha.

But we need to make it flexible because we have many strategies here and we don't push one over the other, let's prevent breaking changes

qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'indices' })
// 'a[0]=b&a[1]=c'
qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'brackets' })
// 'a[]=b&a[]=c'
qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'repeat' })
// 'a=b&a=c'
qs.stringify({ a: ['b', 'c'] }, { arrayFormat: 'comma' })
// 'a=b,c'

cc: @fengmk2 🤔?!

[andrew0]

Please can we not introduce qs to Koa... it's one of the things I wanted to escape from Express. The behavior is very unusual compared to my experience with other web frameworks and makes working with the query object a pain. Any query value could be either a string, an array of strings, or an object, and these arrays/objects can also be nested. So all query values accesses need to be overly defensive, or just call .toString() and accept that sometimes you might be getting [object Object] as a value.

The "bug" that this is fixing doesn't really seem like a problem to me. I think it's reasonable to assume that ctx.request.query will never have any values that are single-value arrays, and that you should always expect that it could be either a string or an array of strings. As far as I can tell, the only situation that this is "fixing" is if you are modifying the request's query object in middleware, setting its value to something that Node's built-in querystring parser would never produce, and you want the rest of your middleware to be able to access ctx.request.query while assuming certain values are an array without checking. Adding non-standard syntax to the query string to support this edge case feels like a hack.