Route inherited param fix #9731
Conversation
|
LGTM for the first commit |
|
This contains also code form my previous pr, I'll fix that later on. |
cmichal
force-pushed
the
routeInheritedParam-fix
branch
from
fc83923 to
254ecc7
Compare
|
Ok, ready. |
| @@ -141,10 +141,13 @@ function $RouteProvider(){ | |||
| * Adds a new route definition to the `$route` service. | |||
| */ | |||
| this.when = function(path, route) { | |||
| var routeCopy = angular.copy(route); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Then again, that's wrong too... so I guess copy is okay, but we should explain clearly why it works.
There was a problem hiding this comment.
Explain in the comment to that code?
cmichal
force-pushed
the
routeInheritedParam-fix
branch
from
254ecc7 to
e0a36c1
Compare
| @@ -141,10 +141,13 @@ function $RouteProvider(){ | |||
| * Adds a new route definition to the `$route` service. | |||
| */ | |||
| this.when = function(path, route) { | |||
| var routeCopy = angular.copy(route); | |||
| if (angular.isUndefined(routeCopy.reloadOnSearch)) { | |||
There was a problem hiding this comment.
if (!('reloadOnSearch' in routeCopy))
copy route params with angular.copy before using angular.extend which looks only for enumerable own properties Closes angular#8181
cmichal
force-pushed
the
routeInheritedParam-fix
branch
from
e0a36c1 to
6bdef8e
Compare
|
how can I resubmit build? current one faild because of some issues with browsers in lab... |
|
restarted it for you. |
|
welp, it crashed again in the same way. |
|
You mean because of lab issues or rather my code issue? |
|
@pkozlowski-opensource do you have any comments on this before I land it? I think we want toget it in shortly |
|
|
||
| it("should use route params inherited from prototype chain", function() { | ||
| var BaseRoute = function BaseRoute() { | ||
| BaseRoute.prototype.templateUrl = 'foo.html'; |
There was a problem hiding this comment.
I would move this out of the constructor...
There was a problem hiding this comment.
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
|
I think i'm just going to make that slight adjustment and land this, we can fix it up later. this doesn't look like it should produce any regressions |
|
murg'd! |
|
(I'm not sure why we have jshint rules that don't let you call |
ngEurope sprint with Vojta & Jeff