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

[update] stringify with comma:true to decode comma for an array as a reserved char per RFC3986 #338

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Om4ar
Copy link
Contributor

@Om4ar Om4ar commented Oct 18, 2019

pr to correctly stringify a array with formatArray as a comma

keeping the comma character decoded to separate items for this character a reserved one per RFC3986

fixes: #337

@ljharb
Copy link
Owner

ljharb commented Oct 23, 2019

When the format is "RFC3986", this seems correct. However (see https://github.com/ljharb/qs#rfc-3986-and-rfc-1738-space-encoding) when the format is RFC1738, for example, i would not expect it to be encoded.

@Om4ar
Copy link
Contributor Author

Om4ar commented Oct 23, 2019

will try to do it for RFC3986 only

Copy link
Owner

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I've rebased this and tweaked some things, but an easy solution hasn't yet presented itself.

} else {
if (encoder) {
var commaKey = encodeValuesOnly ? prefix : encoder(prefix, defaults.encoder, charset, 'key');
var commaValue = obj.map(function (item) {
Copy link
Owner

Choose a reason for hiding this comment

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

we can't add a dependency on .map here.

Choose a reason for hiding this comment

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

what is wrong with the .map?

Copy link
Owner

@ljharb ljharb May 2, 2020

Choose a reason for hiding this comment

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

@petersolopov Pre-ES5 browsers won't have it. The PR can be rebased and use utils.maybeMap instead.

@codecov
Copy link

codecov bot commented Jan 13, 2021

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.08 ⚠️

Comparison is base (da6d249) 99.85% compared to head (9be0025) 99.77%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
- Coverage   99.85%   99.77%   -0.08%     
==========================================
  Files           8        8              
  Lines        1336     1349      +13     
  Branches      164      168       +4     
==========================================
+ Hits         1334     1346      +12     
- Misses          2        3       +1     
Impacted Files Coverage Δ
lib/stringify.js 99.27% <100.00%> (-0.73%) ⬇️
test/stringify.js 99.73% <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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ljharb
Copy link
Owner

ljharb commented Apr 10, 2023

@Om4ar are you no longer interested in completing this PR? Please leave it open even so, so that someone else can complete it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stringify with arrayFormat: comma, wrongfully encode comma in return
3 participants