Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($http): support custom params serializers #11461

Conversation

pkozlowski-opensource
Copy link
Member

@petebacondarwin I still need to add docs but if you could have a look at the impl / tests it would help us iterate faster on this one.

@petebacondarwin
Copy link
Member

I think non string version should be a factory function like interceptors. Otherwise looks good

@petebacondarwin
Copy link
Member

To clarify (I was on phone before):

When defining $http interceptors you specify either the name of a service or a factory function, which can be injected with dependencies.

$httpProvider.interceptors.push(function($q, dependency1, dependency2) {
  return {
   'request': function(config) {
       // same as above
    },

    'response': function(response) {
       // same as above
    }
  };
});

Currently in this PR, when you define a param serializer, you specify either the name of a service or the serializer itself. I am proposing that, to keep parity with the interceptor API, that you must provide a factory function if specifying inline. E.g.

$httpProvider.defaults.paramSerializer = ['someHelperService', function(someHelperService) {
  return function(params) {
  };
}]);

@petebacondarwin
Copy link
Member

One other thing is whether these serializers should be $$ prefixed, which implies that they are private. I guess that in order to fix the issue we need to make these services public, otherwise people wouldn't be able to select them for changing their param serialization.

@jmendiara
Copy link
Contributor

According to jQuery.params doc, objects should be serialized this way

$.param({
  a: { 
    b: 1, 
    c: 2 
  },
  d: [ 3, 4, { e: 5 } ] 
});
// "a[b]=1&a[c]=2&d[]=3&d[]=4&d[2][e]=5"

If you don't want to implement this crazy serialization, maybe $$jqueryParamSerializer name leads to confusion and future issues about object serialization

@pkozlowski-opensource
Copy link
Member Author

@petebacondarwin thnx for the review. Few remarks:

  • I was hesitating between private / non-private. I guess I've marked it as private as not to "commit" to supporting those :-) But I guess you are right - we should make them public
  • function vs. factory function - so, the trouble is that we can also specify functions on the request config level and IMO providing factory functions there would be odd. But I see your point regarding the default config. Shell we make the 2 be not-consistent?

@jmendiara didn't know that jQuery does it to objects :-) I think we can support those without too much pain....

@pkozlowski-opensource
Copy link
Member Author

@petebacondarwin added docs and made serializers public. IMO it is better to leave interactions with DI as-is.

@jmendiara added test + code for handling first-level values as objects. One things I'm not sure about, though, is encoding of keys. Should the new suffixes be encoded as well?

Please have another look at it and let me know if there are any other items that needs to be taken care of before getting this one in.

@jmendiara
Copy link
Contributor

Should the new suffixes be encoded as well?

$.param({a:{'bar/a':'value'}})
//"a%5Bbar%2Fa%5D=value"
decodeURIComponent($.param({a:{'bar/a':'value'}}))
// "a[bar/a]=value"

so... yes 😃

@pkozlowski-opensource
Copy link
Member Author

@jmendiara yeh, this is what I was expecting... will push an update...

var parts = [];
forEachSorted(params, function(value, key) {
if (value === null || isUndefined(value)) return;
if (isArray(value) || isObject(value) && jQueryMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cosmetic: Maybe be explicit here with comparison precedence to have better maintenance?
if (isArray(value) || (isObject(value) && jQueryMode)) {

@petebacondarwin
Copy link
Member

Other than docs tweaks this looks good to me. Thanks @pkozlowski-opensource and @jmendiara

@pkozlowski-opensource
Copy link
Member Author

@petebacondarwin @jmendiara I believe that I've addressed all your comments. Pushed a new commit if you want to have a final look - if I don't hear from you by the end of the day I'm going to land it as-is.

Thank you guys so much for reviewing this one!

@petebacondarwin
Copy link
Member

LGTM

@gkalpak
Copy link
Member

gkalpak commented Apr 14, 2015

@pkozlowski-opensource: Couldn't this also have closed #6039 ? Are there any plans to document this new params serialization thingy (because right now it is as underdocumentd as it gets :)) ?

@pkozlowski-opensource
Copy link
Member Author

@gkalpak #6039 is different - this one is about request param serialization while #6039 is about body serialization. So while we could (?) reuse the same code, it is not the same feature.

And hey, it is documented! Where were you looking for the docs?

@gkalpak
Copy link
Member

gkalpak commented Apr 14, 2015

Oops and oops (first day back from easter holidays - still recovering) 😃

So, it is a different issue, but (if I am not mistaken) jQuery uses the same param method for both, so it seems we could reuse the same code. Now that customizing param serialization is in place, would it take much to implement something similar for body serialization ?
(I still don't get why, but lot's of people would be happy.)

As far as documentation is concerned, it is indeed documented :)
Do you think it would make sense to have a couple more things:

  1. Mention/Link to the built-in serializers (including the default) in $http and $httpProvider.
  2. Maybe add a little section in $http regarding param serialization (like we have for Setting HTTP Headers, Transforming Requests and Responses, Caching etc).

@jmendiara
Copy link
Contributor

@pkozlowski-opensource: I've created a gist with some $http related issues. Can you take a look to https://gist.github.com/jmendiara/55cb370a9514e969f0f3 ?

@pkozlowski-opensource
Copy link
Member Author

@jmendiara gist is not the most effective in raising issues. Please open a separate ticket for each problem (with a reproduce scenario!). But before doing so, please check that an issue wan't open - https://github.com/angular/angular.js/issues?q=is%3Aopen+is%3Aissue+label%3A%22component%3A+%24http%22 - I'm pretty sure that there are issues already open for most of the items from your list (especially around caching)

netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@michaelbudnik
Copy link

@pkozlowski-opensource
I would like to see similar feature for $location.search(). There is no way to change the serialization process of the params in url. My specific problem is that I would like "+" not to be encoded as "%20" , but there is no way of changing this.

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

Successfully merging this pull request may close these issues.

None yet

6 participants