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

Square brackets should be URI encoded #8

Closed
wants to merge 1 commit into from
Closed

Square brackets should be URI encoded #8

wants to merge 1 commit into from

Conversation

porsager
Copy link

@porsager porsager commented Aug 6, 2014

No description provided.

@nlf
Copy link
Collaborator

nlf commented Aug 6, 2014

This would be a breaking change and a drastic deviation from the original qs library. As you can see, it breaks quite a few of the stringify tests. This is definitely one that requires more discussion before being merged.

@nlf nlf self-assigned this Aug 6, 2014
@porsager
Copy link
Author

porsager commented Aug 6, 2014

Yeah, sorry for not looking through the tests!
As if it's a drastic change, i'm not really sure. Using qs itself to parse the new resulting string works fine.
I can't really figure out where it could break existing systems since the resulting string needs to be decoded anyway. Could you think of any?

@nlf
Copy link
Collaborator

nlf commented Aug 6, 2014

I'm not sure, but I'll do a little testing. I think there's a better place to do the encoding if it's necessary, too.

nlf added a commit that referenced this pull request Aug 6, 2014
@nlf
Copy link
Collaborator

nlf commented Aug 6, 2014

Closed via 5b63418

@nlf nlf closed this Aug 6, 2014
Morita0711 added a commit to Morita0711/npm-superurgent that referenced this pull request Jul 3, 2022
qs had the following breaking changes:
- Square brackets get urlencoded (see: ljharb/qs#8)
- Use date.toISOString() for date serialization (see: ljharb/qs@e473156)
Also update better-assert
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.

None yet

2 participants