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.

Sign up for GitHub

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

Jump to bottom

Add arrayFormat option #70

Merged
merged 1 commit into from
Feb 18, 2015
Merged

Add arrayFormat option #70

merged 1 commit into from
Feb 18, 2015

Conversation

mjackson
Copy link
Contributor

The arrayFormat option recognizes 3 values: brackets, indices, and
repeat. It defaults to bracket indices.

See the comments in #46

mjackson referenced this pull request in remix-run/react-router Feb 17, 2015
@mjackson
[fixed] Double-encoding of query strings
fe5ec39 
Also, don't use indices in array query string values.

Fixes #721
@nlf
Copy link
Collaborator

nlf commented Feb 17, 2015

This would be a breaking change, but I think we can tweak it to not be breaking fairly easily..

Change the default if arrayFormat is not supplied to be indices, which lines up with old behavior.

Allow passing indices as an option and if it is set to false, set the generateArrayPrefix to repeat, that way that behavior continues working as before.

@nlf nlf self-assigned this Feb 17, 2015
@nlf
Copy link
Collaborator

nlf commented Feb 17, 2015

Note that with those tweaks, I'd feel better if you left the old existing tests in place and only added new ones so we can be certain we're not breaking anything.

@mjackson
Copy link
Contributor Author

@nlf I thought this change might be worth breaking backwards compat instead of having two separate options for specifying the same thing. e.g. if you use both the old indices option and the new arrayFormat option, which one takes precedence?

IMHO it would be cleaner to just say "if you're using >=2.4 use arrayFormat". But if you'd still like to preserve compat with the indices option I'll happily keep it around. arrayFormat would take precedence though.

@mjackson
Copy link
Contributor Author

@nlf I updated the PR so the default is indices though I'd still like to break backwards compat if you'd be game.

@nlf
Copy link
Collaborator

nlf commented Feb 17, 2015

I'd definitely prefer to not make this a breaking change just yet. I think we document the indices option as deprecated and that arrayFormat takes precedence though.

I have a few other things I intend to work on for the next major version bump, so when that stuff is complete is when we'll remove the indices option completely.

@mjackson
Add arrayFormat option
9c46a95 
This commit adds the arrayFormat option to qs.stringify. Valid
values are "indices" (the default), "repeat", or "brackets".

Backwards compat is preserved with the indices option as well.
@mjackson
Copy link
Contributor Author

@nlf Gotcha. I've updated the PR to support the indices option. All existing tests pass unmodified. Only new tests :)

@mjackson
Copy link
Contributor Author

BTW, it looks like this may supersede #65

@nlf
Copy link
Collaborator

nlf commented Feb 18, 2015

Perfect! This looks great, thanks!

@nlf nlf added this to the 2.4.0 milestone Feb 18, 2015
nlf added a commit that referenced this pull request Feb 18, 2015
@nlf
Merge pull request #70 from mjackson/array-format
70f35dd 
Add arrayFormat option
@nlf nlf merged commit 70f35dd into ljharb:master Feb 18, 2015
@nlf nlf mentioned this pull request Feb 18, 2015
Closed
@mjackson
Copy link
Contributor Author

Awesome. Thanks for being so responsive @nlf ! :)

@simov simov mentioned this pull request Mar 12, 2015
Closed
@simov
Copy link
Contributor

simov commented Mar 12, 2015

Hi, can we get this version published on npm in the near future? We have similar feature request here request/request#1473

@nlf
Copy link
Collaborator

nlf commented Mar 12, 2015

Just published 2.4.0, I had it all primed and everything and just never hit enter. Sorry about that!

@simov
Copy link
Contributor

simov commented Mar 12, 2015

Thanks 👍

@simov simov mentioned this pull request Mar 13, 2015
Merged
@peril-staging peril-staging bot mentioned this pull request Jan 15, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nlf nlf

Labels
enhancement
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mjackson @nlf @simov