Encode comma values more consistently

[bakkerthehacker]

Fixes #459 but only for encodeValuesOnly: true

This PR includes tests for arrays with comma values and different settings.

The behaviour for encodeValuesOnly has been adjusted to encode values before they are joined together, as an example it changes stringifying { a: [',', '', 'c,d'] } from a=,,,c,d to a=%2C,,c%2Cd.

The behaviour of the other 2 encoding setups (encode: false) and (encode: true with encodeValuesOnly: false) have been left unmodified. These formats do seem less useful for arrayFormat: 'comma', and tricky to get right in all situations.

However, there is additional functionality in stringify that isnt really tested, but is related to these options for the types:
'string' 'number' 'boolean' 'symbol' 'bigint' 'buffer'. This inconsistent additional functionality also avoids encoding commas when they appear in these types.

I have included additional tests for these situations, where a comma appears in a string outside an array. I have also removed the custom functionality in this PR and these types now encode comma values normally. If this change goes too far, it can be removed, but it seems odd to have the comma version of the arrayFormat affect non-array types just because they contain a comma.

[coatesap]

This looks great, and would definitely allow us to finally start using arrayFormat in production.

[ljharb]

This looks good overall, but there's a test failing - "array with multiple items with a comma inside". Does that mean this is a breaking change, or is it a bug in the PR?

[bakkerthehacker]

This looks good overall, but there's a test failing - "array with multiple items with a comma inside". Does that mean this is a breaking change, or is it a bug in the PR?

There were already tests for 2 of the 12 test cases that I added here. Both the existing test cases use arrayFormat: comma which is being adjusted in this PR. One of the 2 tests has a double encode (comma -> %2C -> %25%2C) which seems wrong, and it is affected by the change.

[ljharb]

Given that qs.parse('a=c%25%2Cd%2Ce') is { a: 'c%,d,e' } and qs.parse('a=c%2Cd%2Ce') is { a: 'c,d,e' }, this is certainly an improvement, but i think the ideal end point is where things can round trip.

I'll consider this semver-patch since for comma arrayFormat users it's clearly broken atm.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change

Comparison is base (20820fa) 99.79% compared to head (4c4b23d) 99.79%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #463   +/-   ##
=======================================
  Coverage   99.79%   99.79%           
=======================================
  Files           8        8           
  Lines        1459     1487   +28     
  Branches      177      176    -1     
=======================================
+ Hits         1456     1484   +28     
  Misses          3        3           

Impacted Files	Coverage Δ
lib/stringify.js	99.35% <100.00%> (-0.02%)	⬇️
test/stringify.js	99.78% <100.00%> (+0.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.