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

Route inherited param fix #9731

Closed
wants to merge 1 commit into from
Closed

Route inherited param fix #9731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions src/ngRoute/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,14 @@ function $RouteProvider(){
* Adds a new route definition to the `$route` service.
*/
this.when = function(path, route) {
//copy original route object to preserve params inherited from proto chain
var routeCopy = angular.copy(route);
Copy link
Contributor

Choose a reason for hiding this comment

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

it's weird that we're doing the copy() and extend() ... the reason for the copy() is just for the Object.create(Object.getPrototypeOf(route)) --- but it takes some digging to figure out why it works.

How about this:

route = Object.create(Object.getPrototypeOf(route));
if (!'reloadOnSearch' in route) route.reloadOnSearch = true;
routes[path] = angular.extend(routeCopy, path && pathRegExp(path, route));

That way it's clearer why it actually works

Copy link
Contributor

Choose a reason for hiding this comment

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

Then again, that's wrong too... so I guess copy is okay, but we should explain clearly why it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Explain in the comment to that code?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, add a comment

if (angular.isUndefined(routeCopy.reloadOnSearch)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!('reloadOnSearch' in routeCopy))

routeCopy.reloadOnSearch = true;
}
routes[path] = angular.extend(
{reloadOnSearch: true},
route,
path && pathRegExp(path, route)
routeCopy,
path && pathRegExp(path, routeCopy)
);

// create redirection for trailing slashes
Expand All @@ -155,7 +159,7 @@ function $RouteProvider(){

routes[redirectPath] = angular.extend(
{redirectTo: path},
pathRegExp(redirectPath, route)
pathRegExp(redirectPath, routeCopy)
);
}

Expand Down
17 changes: 17 additions & 0 deletions test/ngRoute/routeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,23 @@ describe('$route', function() {
$rootScope.$digest();
expect($route.current).toBeDefined();
}));

it("should use route params inherited from prototype chain", function() {
var BaseRoute = function BaseRoute() {
BaseRoute.prototype.templateUrl = 'foo.html';
Copy link
Contributor

Choose a reason for hiding this comment

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

doing this in the constructor is weird

Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this out of the constructor...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't care a whole lot about this in a test, but I suppose we should take this out of the constructor in case anyone ever wants to copy/paste from it

};
var route = new BaseRoute();

module(function($routeProvider) {
$routeProvider.when('/foo', route);
});

inject(function($route, $location, $rootScope) {
$location.path('/foo');
$rootScope.$digest();
expect($route.current.templateUrl).toBe('foo.html');
});
});
});


Expand Down