-
Notifications
You must be signed in to change notification settings - Fork 27.4k
[WIP] feat($httpUrlParams): introduce new service abstracting params serialization #11238
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
||
|
@@ -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); | ||
|
@@ -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); | ||
if (params) { | ||
url += ((url.indexOf('?') == -1) ? '?' : '&') + params; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While you are touching this, |
||
} | ||
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 ''; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 BTW, you are using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should ditch the |
||
}; | ||
|
||
return HttpUrlParams; | ||
}; | ||
|
||
this.registerSerializer(this.defaultMode, function(params) { | ||
return serializeParams(params, false); | ||
}); | ||
|
||
this.registerSerializer('jquery', function(params) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this the same as the default ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||
return serializeParams(params, false); | ||
}); | ||
} |
There was a problem hiding this comment.
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.