fix(ngRoute): Don't deep-copy the route object, just plain copy it instead. #14699
Conversation
This seems to fix angular#14478. No need for a deep copy, just a plain one is enough as ngRoute does not modify the fields in the route object.
Forgot a comma.
|
The question is why was the original |
|
The commit introducing the code is this: b477058, where they switched extending an object with route (without the inherited components of route) to extending routeCopy. Their test still pass, and my way will keep the original prototype chain of the route object. |
|
I'm talking about the commit that introduced the previous implementation, using |
|
In it's current form, it seems to come from here: 15ecc6f Note the comment in the changed version that talks about merging routes |
|
It seems it has been using I agree that taking into account inherited properties is a more common usecase than extending a route, but ideally we should provide a way to support both. |
| @@ -160,7 +160,7 @@ function $RouteProvider() { | |||
| */ | |||
| this.when = function(path, route) { | |||
| //copy original route object to preserve params inherited from proto chain | |||
| var routeCopy = angular.copy(route); | |||
| var routeCopy = route; | |||
There was a problem hiding this comment.
I believe that the intention of the copying the object was to avoid the possibility of the user mutating the passed in object after the router partially processed it. This could get the router into an unknown state.
By copying we know for sure that the state we read has not changed.
The problem this causes is that if the templateUrl is set to be a TrustedResourceUrl instance, the instance is not copied correctly and the templateUrl fails to be properly unwrapped by sce later.
Since both template and templateUrl properties are both (wrapped) immutable strings, we could use shallowCopy instead to protect the router from internal state corruption while not corrupting the sce wrappers.
Another option is to teach angular.copy how to copy sce wrappers safely, but given that there could be custom implementations of these wrappers (e.g. at google we have our own), making the the mechanism generic might be tricky.
shallowCopy seems to give us all of the benefits with little down-sides, so I'd prefer that one.
There was a problem hiding this comment.
I think the intention of using copy instead of extend was to preserve inherited properties (talk about using a sledgehammer to crack a nut 😃). shallowCopy (which we need to copy over to angular-route btw) would also shallow-copy inherited properties, so we are covered.
Deep-copying route definition objects can break specific custom implementations of `$sce` (used to trust a `templateUrl` as RESOURCE_URL. The purpose of copying route definition objects was to guard against the user's modifying the route definition object after route registration, while still capturing inherited properties. As suggested by @IgorMinar in angular#14699 (comment), we can achieve both _and_ support custom `$sce` implementations, by shallow-copying instead. This is an alternative implementation for angular#14699, which avoids the breaking change. Fixes 14478 Closes 14699
Deep-copying route definition objects can break specific custom implementations of `$sce` (used to trust a `templateUrl` as RESOURCE_URL). The purpose of copying route definition objects was to guard against the user's modifying the route definition object after route registration, while still capturing inherited properties. As suggested by @IgorMinar in angular#14699 (comment), we can achieve both _and_ support custom `$sce` implementations, by shallow-copying instead. This is an alternative implementation for angular#14699, which avoids the breaking change. Fixes 14478 Closes 14699
Deep-copying route definition objects can break specific custom implementations of `$sce` (used to trust a `templateUrl` as RESOURCE_URL). The purpose of copying route definition objects was to guard against the user's modifying the route definition object after route registration, while still capturing inherited properties. As suggested by @IgorMinar in #14699 (comment), we can achieve both _and_ support custom `$sce` implementations, by shallow-copying instead. This is an alternative implementation for #14699, which avoids the breaking change. Fixes #14478 Closes #14699 Closes #14750
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix for #14478
What is the current behavior? (You can also link to an open issue here)
Angular runs angular.copy on its routes, which isn't necessary and breaks some things
What is the new behavior (if this is a feature change)?
No more angular.copy on these, which appears to not break anything.
Does this PR introduce a breaking change?
No, it shouldn't.
Please check if the PR fulfills these requirements
That's Angular's guts, the previous behavior wasn't documented, and the new one seems obvious enough.
Other information:
This seems to fix #14478. No need for a deep copy, just a plain one is enough as ngRoute does not modify the fields in the route object.
I'm not that familiar with ngRoute, and the fix seems too easy: if something's wrong, I'll be glad to add tests for that :) As mentionned in the issue, #8181 and #9731 seems to be relevant, but this PR shouldn't reopen the issues.