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

improve interpolation of params in ngRoute #8200

Closed
wants to merge 2 commits into from
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
23 changes: 12 additions & 11 deletions src/ngRoute/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -606,19 +606,20 @@ function $RouteProvider(){
* @returns {string} interpolation of the redirect path with the parameters
*/
function interpolate(string, params) {
var result = [];
angular.forEach((string||'').split(':'), function(segment, i) {
if (i === 0) {
result.push(segment);
return (string||'').replace(/(\/)?:(\w+)([?*])?/g, interpolateReplacer);

function interpolateReplacer(_, slash, key, option) {
var optional = option === '?';
var value = params[key];
delete params[key];

if (optional && value == null) {
return '';
} else {
var segmentMatch = segment.match(/(\w+)(.*)/);
var key = segmentMatch[1];
result.push(params[key]);
result.push(segmentMatch[2] || '');
delete params[key];
slash = slash || '';
return slash + (value || '');
}
});
return result.join('');
}
}
}];
}
41 changes: 41 additions & 0 deletions test/ngRoute/routeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,47 @@ describe('$route', function() {
});


it('should interpolate optional route vars in the redirected path from original path', function() {
module(function($routeProvider) {
$routeProvider.when('/foo/:id/foo/:subid?/:extraId', {redirectTo: '/bar/:id/:subid?/23'});
$routeProvider.when('/bar/:id/:subid?/:subsubid', {templateUrl: 'bar.html'});
});

inject(function($route, $location, $rootScope) {
$location.path('/foo/id1/foo/subid3/gah');
$rootScope.$digest();

expect($location.path()).toEqual('/bar/id1/subid3/23');
expect($location.search()).toEqual({extraId: 'gah'});
expect($route.current.params).toEqual({id:'id1', subid:'subid3', subsubid:'23', extraId:'gah'});
expect($route.current.templateUrl).toEqual('bar.html');

$location.path('/foo/id1/foo/gah');
$rootScope.$digest();

expect($location.path()).toEqual('/bar/id1/23');
expect($location.search()).toEqual({extraId: 'gah'});
Copy link
Contributor

Choose a reason for hiding this comment

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

should test that $route.current.params equal what we expect, not just $location.search() (for each of these cases).

expect($route.current.params).toEqual({id:'id1', subsubid:'23', extraId:'gah'});
});
});


it('should interpolate catch-all route vars in the redirected path from original path', function() {
module(function($routeProvider) {
$routeProvider.when('/baz/:id/:path*', {redirectTo: '/path/:path*/:id'});
$routeProvider.when('/path/:path*/:id', {templateUrl: 'foo.html'});
});

inject(function($route, $location, $rootScope) {
$location.path('/baz/1/foovalue/barvalue');
$rootScope.$digest();
expect($location.path()).toEqual('/path/foovalue/barvalue/1');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also split this test apart, because this is not testing just one unit of functionality. It would be better to have separate test cases for each of the things being tested here.

expect($route.current.params).toEqual({id:'1', path:'foovalue/barvalue'});
expect($route.current.templateUrl).toEqual('foo.html');
});
});


it('should interpolate route vars in the redirected path from original search', function() {
module(function($routeProvider) {
$routeProvider.when('/bar/:id/:subid/:subsubid', {templateUrl: 'bar.html'});
Expand Down