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

[WIP] feat($httpUrlParams): introduce new service abstracting params serialization #11238

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/AngularPublic.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
$IntervalProvider,
$HttpProvider,
$HttpBackendProvider,
$HttpUrlParamsProvider,
$LocationProvider,
$LogProvider,
$ParseProvider,
Expand Down Expand Up @@ -224,6 +225,7 @@ function publishExternalAPI(angular) {
$interval: $IntervalProvider,
$http: $HttpProvider,
$httpBackend: $HttpBackendProvider,
$httpUrlParams: $HttpUrlParamsProvider,
$location: $LocationProvider,
$log: $LogProvider,
$parse: $ParseProvider,
Expand Down
105 changes: 81 additions & 24 deletions src/ng/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,8 @@ function $HttpProvider() {
**/
var interceptorFactories = this.interceptors = [];

this.$get = ['$httpBackend', '$$cookieReader', '$cacheFactory', '$rootScope', '$q', '$injector',
function($httpBackend, $$cookieReader, $cacheFactory, $rootScope, $q, $injector) {
this.$get = ['$httpBackend', '$httpUrlParams', '$$cookieReader', '$cacheFactory', '$rootScope', '$q', '$injector',
function($httpBackend, $httpUrlParams, $$cookieReader, $cacheFactory, $rootScope, $q, $injector) {

var defaultCache = $cacheFactory('$http');

Expand Down Expand Up @@ -1028,7 +1028,7 @@ function $HttpProvider() {
cache,
cachedResp,
reqHeaders = config.headers,
url = buildUrl(config.url, config.params);
url = buildUrl(config.url, config.params, config.paramsMode);

$http.pendingRequests.push(config);
promise.then(removePendingReq, removePendingReq);
Expand Down Expand Up @@ -1135,29 +1135,86 @@ function $HttpProvider() {
}


function buildUrl(url, params) {
if (!params) return url;
var parts = [];
forEachSorted(params, function(value, key) {
if (value === null || isUndefined(value)) return;
if (!isArray(value)) value = [value];

forEach(value, function(v) {
if (isObject(v)) {
if (isDate(v)) {
v = v.toISOString();
} else {
v = toJson(v);
}
}
parts.push(encodeUriQuery(key) + '=' +
encodeUriQuery(v));
});
});
if (parts.length > 0) {
url += ((url.indexOf('?') == -1) ? '?' : '&') + parts.join('&');
function buildUrl(url, params, mode) {
params = $httpUrlParams.serialize(params, mode);
Copy link
Contributor

Choose a reason for hiding this comment

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

For maxium flexibility, how about we allow mode to be a function? So you could have an inline params converter in your http config.

if (params) {
url += ((url.indexOf('?') == -1) ? '?' : '&') + params;
Copy link
Member

Choose a reason for hiding this comment

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

While you are touching this, === would be nice :)

}
return url;
}
}];
}

/**
* @ngdoc provider
* @name $httpUrlParamsProvider
* @description
* Use `$httpUrlParamsProvider` to change the default behavior of the {@link ng.$httpUrlParams $httpUrlParams} service.
* */
function $HttpUrlParamsProvider() {

var paramSerializers = createMap();

function serializeParams(params, addArrayMarker) {
var parts = [];

if (!params) return '';
Copy link
Member

Choose a reason for hiding this comment

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

Pretty minor, but why not move this to the top (to avoid unused array creation) ?


forEachSorted(params, function(value, key) {
if (value === null || isUndefined(value)) return;
if (!isArray(value)) value = [value];

forEach(value, function(v) {
if (isObject(v)) {
v = isDate(v) ? v.toISOString() : toJson(v);
}
parts.push(encodeUriQuery(key) + (addArrayMarker ? '[]' : '') + '=' + encodeUriQuery(v));
Copy link
Member

Choose a reason for hiding this comment

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

I feel like deciding whether to add an array-marker or not, should somehow be more "clever" (e.g. take into account if value was initially an array or something).
With the current implementation, it seems to be a pain to add a marker to certain params.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree we should only be adding an array marker to params that are actually arrays.

});
});

return parts.join('&');
}


this.defaultMode = 'traditional';


this.registerSerializer = function registerSerializer(mode, serFn) {
paramSerializers[mode] = serFn;
};

/**
* @ngdoc service
* @name $httpUrlParams
*
* @description
* The `$httpUrlParams` service is responsible for serializing request params
* (expressed as a JavaScript object) to a string.
* It abstracts the way in which request params are transformed, grouped, encoded etc.
*
* Normally you wouldn't use this service directly in the application's code but rather override this service
* to enable custom serialization schemas for URL parameters.
*
* The `$httpUrlParams` service comes handy while unit testing with {@link ngMock.$httpBackend $httpBackend mock},
* as one can inject `$httpUrlParams` into a test and serialize URL params as {@link ng.$http $http} service.
*/
this.$get = function() {

var provider = this;
var HttpUrlParams = {};

HttpUrlParams.serialize = function(params, mode) {
return paramSerializers[lowercase(mode) || provider.defaultMode](params);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we throw if a mode is not available? I think we should.

Copy link
Member

Choose a reason for hiding this comment

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

If we decide to throw, it would be better to throw a more meaningful error (instead of undefined is not a function).
I would also consider logging the incident and falling back to the default mode.

BTW, you are using lowercase here, but not when registering. They should be consistent (and probably documented).

Copy link
Contributor

Choose a reason for hiding this comment

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

We should ditch the lowercase altogether. People should be able to specify the correct capitalization.

};

return HttpUrlParams;
};

this.registerSerializer(this.defaultMode, function(params) {
return serializeParams(params, false);
});

this.registerSerializer('jquery', function(params) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the same as the default ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be return serializeParams(params, true)?

return serializeParams(params, false);
});
}