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

Fixing arrays in get params #49

Merged
merged 3 commits into from Jul 23, 2015

Conversation

@maxhoffmann
Copy link
Contributor

commented Mar 17, 2015

I had an issue sending arrays via GET params to a Rails server. In the current implementation the key in the params does not contain square brackets so that the server doesn’t know that it should handle the incoming params as an array. It only takes the last one.

There already is a test that checks for the implementation without square brackets. Is this defined as a standard somewhere? I researched on some sites and apparently PHP and Ruby’s Rack only support the square bracket notation. As transformRequest does only work for data and also not for GET requests I can’t override this functionality via the axios API. It seems that this has to be handled internally.

I’m proposing to change the current implementation so that square brackets are used for array params in GET requests. What do you think?

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2015

Any opinion on this one?

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Mar 23, 2015

I'm doing some research to determine if there is a standard.

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2015

Cool, thanks. 👍

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2015

Any update on this? I’m currently using my fixed version directly from github. Would be nice to merge this in. :)

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2015

According to my research Rails and PHP only support the bracket notation.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Mar 31, 2015

I think that bracket notation is the correct implementation. It looks like rails, express, php, and java support it. My only hangup is if there should be an option for using one vs the other. This change won't be backward compatible for anyone depending on ?foo=bar&foo=baz.

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2015

I could add an option so that people can enable it if the change breaks something. Although I’d say increasing the major version and adding a line to changelog should be sufficient.

In case you’d add an option: how would you name and add the config parameter?

axios({
  disableGetBracketNotation: true
});

I think bracket notation should be the default, so the option should fall back to the old behavior.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Apr 8, 2015

I've been thinking and it may be better instead of just using a boolean flag, allow specifying a function for how to handle params. This would accommodate any variety of patterns: foo[]=bar&foo[]=baz, foo=bar&foo=baz, foo=bar,baz, etc.

There are a couple options for doing this.

  1. Allow developer to provide a buildUrl method in the config. This would replace the internal buildUrl method if supplied. This could be done per request, or set as the default axios.defaults.buildUrl = function (url, params) { ... };
  2. Allow providing a method that just builds out the params, which would be used by buildUrl. Something like axios.defaults.processParams = function (params) { ... }; This would allow re-using more of the core logic, but isn't as flexible.
@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2015

Yep, passing a function sounds better. So would you move buildUrl to defaults.js and make it a config option like transformRequest?

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Apr 14, 2015

Yeah, that's basically what I'm thinking. The thing I'm debating is that there's a bit of other logic in buildUrl that it may be nice to preserve. Specifically the encode function keeps the params looking nice.

Maybe buildUrl could take encode as an argument. Then implementing your own buildUrl could still leverage this functionality. Let me play around with this a bit, and see what makes sense.

@maxhoffmann maxhoffmann force-pushed the maxhoffmann:master branch from 8337a40 to 4e2b588 Apr 20, 2015

@webbushka

This comment has been minimized.

Copy link

commented Jun 1, 2015

Any decisions made on this?

@renfredxh

This comment has been minimized.

Copy link

commented Jun 13, 2015

I'm using a Python backend and ran into this issue. An extensible buildUrl function would be nice, but I think the bracket notation is such a wide use case that it would be useful to have a boolean flag option for it. jQuery's $.get enables bracket encoding by default and many people are used to having it available.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Jun 19, 2015

I have a branch that supports providing your own buildUrl, but I don't love it. Another alternative would be to support params as a string, then you could use qs, or whatever you want, to format params how your backend expects. See #72

@AndrewRayCode

This comment has been minimized.

Copy link

commented Jul 13, 2015

Also hoping for a fix. An ajax library that incorrectly serializes an array is not good!

FYI express's req.query also expects the key[]=val notation

@AndrewRayCode

This comment has been minimized.

Copy link

commented Jul 14, 2015

For anyone hitting this, I made a wrapper around axios that is 100% untested and definitely only suits the bare minimum of my needs, but it addresses this issue.

It assumes a function where you pass in params (query string params) as an object, and the method, and the url

//  see https://github.com/mzabriskie/axios/pull/49
if( method.toLowerCase() === 'get' ) {
    for( let key in params ) {
        if( _.isArray( params[ key ] ) ) {
            url += '?' + key + '[]=' + _.map( params[ key ], encodeURIComponent ).join( '&' + key + '[]=' );
            delete params[ key ];
        }
    }
}
@mzabriskie

This comment has been minimized.

Copy link
Member

commented Jul 23, 2015

Sorry for the delay on this. A little background on the current implementation. The original version of axios was meant to provide an API directly compatible with Angular's $http service. The way that axios is sending Array params is how Angular is doing it.

In the time since, axios has diverged in several ways. I was initially pretty set on not breaking backwards compatibility, but after stewing on it, and seeing that most (all?) popular backends support [] for array params, I think it's best to just make a breaking change, and bump the version.

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Jul 23, 2015

@maxhoffmann I would like to merge this. Can you make a couple quick changes?

lib/helpers/buildUrl.js:12
Add the following lines at the end of the encode function:

  replace(/%5B/gi, '[').
  replace(/%5D/gi, ']');

test/specs/helpers/buildUrl.spec.js:33
Change the expect to the following:

  toEqual('/foo?foo[]=bar&foo[]=baz');

This will make the URLs look a bit nicer. Thanks!

maxhoffmann added some commits Jul 23, 2015

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2015

Sure, changes are done. Looking forward to remove my fork from our package.json. 👍

mzabriskie added a commit that referenced this pull request Jul 23, 2015

Merge pull request #49 from maxhoffmann/master
Fixing arrays in get params

@mzabriskie mzabriskie merged commit 3b10b6a into axios:master Jul 23, 2015

1 of 2 checks passed

coverage/coveralls Coverage pending from Coveralls.io
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2015

Thanks for merging! When is the next release?

@iam4x

This comment has been minimized.

Copy link

commented Jul 28, 2015

bump, waiting on this too 👍

@liverbool

This comment has been minimized.

Copy link

commented Aug 2, 2015

👍

@maxhoffmann

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2015

any plans for bumping the version number?

@mzabriskie

This comment has been minimized.

Copy link
Member

commented Sep 21, 2015

@maxhoffmann sorry for the long delay on this. I just released 0.6.0 which includes your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.