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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@pkozlowski-opensource
Member

pkozlowski-opensource commented Mar 30, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 30, 2015

Member

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

Member

petebacondarwin commented Mar 30, 2015

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

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 30, 2015

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) {
  };
}]);
Member

petebacondarwin commented Mar 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Mar 30, 2015

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.

Member

petebacondarwin commented Mar 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jmendiara

jmendiara Mar 31, 2015

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

Contributor

jmendiara commented Mar 31, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Mar 31, 2015

Member

@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....

Member

pkozlowski-opensource commented Mar 31, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Apr 1, 2015

Member

@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.

Member

pkozlowski-opensource commented Apr 1, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jmendiara

jmendiara Apr 1, 2015

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 😃

Contributor

jmendiara commented Apr 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Apr 1, 2015

Member

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

Member

pkozlowski-opensource commented Apr 1, 2015

@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) {

This comment has been minimized.

@jmendiara

jmendiara Apr 1, 2015

Contributor

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

@jmendiara

jmendiara Apr 1, 2015

Contributor

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

Show outdated Hide outdated src/ng/http.js
}
/**
* @ngdoc provider

This comment has been minimized.

@petebacondarwin

petebacondarwin Apr 1, 2015

Member

Don't document the provider as it doesn't do anything. We should be documenting the service here. I.E. Change the tag to @ngdoc service and the name to $httpParamSerializer and move the comment block down to above the $get method below.

Same for the other serializer

@petebacondarwin

petebacondarwin Apr 1, 2015

Member

Don't document the provider as it doesn't do anything. We should be documenting the service here. I.E. Change the tag to @ngdoc service and the name to $httpParamSerializer and move the comment block down to above the $get method below.

Same for the other serializer

Show outdated Hide outdated src/ng/http.js
@@ -153,6 +211,11 @@ function $HttpProvider() {
* - **`defaults.headers.put`**
* - **`defaults.headers.patch`**
*
* - **`defaults.paramSerializer`** - {string|function([paramsObject])} - A function used to prepare string representation

This comment has been minimized.

@petebacondarwin

petebacondarwin Apr 1, 2015

Member

I think the type should be {string|function(Object<string,string>):string}, no?

@petebacondarwin

petebacondarwin Apr 1, 2015

Member

I think the type should be {string|function(Object<string,string>):string}, no?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Apr 1, 2015

Member

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

Member

petebacondarwin commented Apr 1, 2015

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

@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Apr 2, 2015

Member

@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!

Member

pkozlowski-opensource commented Apr 2, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin
Member

petebacondarwin commented Apr 2, 2015

LGTM

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Apr 14, 2015

Member

@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 :)) ?

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

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Apr 14, 2015

Member

@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?

Member

pkozlowski-opensource commented Apr 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Apr 14, 2015

Member

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).
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

This comment has been minimized.

Show comment
Hide comment
@jmendiara

jmendiara Apr 28, 2015

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 ?

Contributor

jmendiara commented Apr 28, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Apr 28, 2015

Member

@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)

Member

pkozlowski-opensource commented Apr 28, 2015

@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 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

@michaelbudnik

This comment has been minimized.

Show comment
Hide comment
@michaelbudnik

michaelbudnik Jan 29, 2016

@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.

michaelbudnik commented Jan 29, 2016

@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 join this conversation on GitHub. Already have an account? Sign in to comment