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

fix($routeProvider): do not deep-copy route definition objects #14750

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
13 changes: 12 additions & 1 deletion src/ngRoute/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ function $RouteProvider() {
return angular.extend(Object.create(parent), extra);
}

function createShallowCopy(src) {
if (!angular.isObject(src)) return src;

var dst = {};
for (var key in src) {
dst[key] = src[key];
}

return dst;
}

var routes = {};

/**
Expand Down Expand Up @@ -160,7 +171,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 = createShallowCopy(route);
Copy link
Contributor

Choose a reason for hiding this comment

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

can't you use this one:

function shallowCopy(src, dst) {
?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are in a external module, we need to copy it. I started by copying that, but then removed the stuff that didn't seem relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. sgtm.

On Thu, Jun 9, 2016 at 2:25 PM Georgios Kalpakas [email protected]
wrote:

In src/ngRoute/route.js
#14750 (comment):

@@ -160,7 +171,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 = createShallowCopy(route);

Since we are in a external module, we need to copy it. I started by
copying that, but then removed the stuff that didn't seem relevant.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/angular/angular.js/pull/14750/files/f4b882c8c30d0d0d68fe04ed65bfca1640cfe0cf#r66524807,
or mute the thread
https://github.com/notifications/unsubscribe/AANM6M5AG48ivz-eHgiQTGEgpn15Qbnpks5qKITJgaJpZM4IyaSo
.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this another case where we should split the shared function into its own file so that we can reuse it in multiple modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but I wouldn't.

We don't need the exact same function. The one from core can shallow-copy arrays as well, ignores $$ prefixed properties and supports passing your own dst object, which properties are shallow-copied onto. We we don't need any of that.

if (angular.isUndefined(routeCopy.reloadOnSearch)) {
routeCopy.reloadOnSearch = true;
}
Expand Down
83 changes: 82 additions & 1 deletion test/ngRoute/routeSpec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

describe('$route', function() {
fdescribe('$route', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

fdescribe

var $httpBackend,
element;

Expand Down Expand Up @@ -900,6 +900,87 @@ describe('$route', function() {
});


it('should not get affected by modifying the route definition object after route registration',
function() {
module(function($routeProvider) {
var rdo = {};

rdo.templateUrl = 'foo.html';
$routeProvider.when('/foo', rdo);

rdo.templateUrl = 'bar.html';
$routeProvider.when('/bar', rdo);
});

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

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


it('should use the property values of the passed in route definition object directly',
function() {
var $routeProvider;

module(function(_$routeProvider_) {
$routeProvider = _$routeProvider_;
});

inject(function($location, $rootScope, $route, $sce) {
var sceWrappedUrl = $sce.trustAsResourceUrl('foo.html');
$routeProvider.when('/foo', {templateUrl: sceWrappedUrl});
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the $routeProvider registration in the inject and not the module block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I couldn't access $sce during config 😟


$location.path('/foo');
$rootScope.$digest();
expect($location.path()).toBe('/foo');
expect($route.current.templateUrl).toBe(sceWrappedUrl);
});
}
);


it('should support custom `$sce` implementations', function() {
function MySafeResourceUrl(val) {
var self = this;
this._val = val;
this.getVal = function() {
return (this !== self) ? null : this._val;
};
}

var $routeProvider;

module(function($provide, _$routeProvider_) {
$routeProvider = _$routeProvider_;

$provide.decorator('$sce', function($delegate) {
$delegate.trustAsResourceUrl = function(url) { return new MySafeResourceUrl(url); };
$delegate.getTrustedResourceUrl = function(v) { return v.getVal(); };
$delegate.valueOf = function(v) { return v.getVal(); };
return $delegate;
});
});

inject(function($location, $rootScope, $route, $sce) {
$routeProvider.when('/foo', {templateUrl: $sce.trustAsResourceUrl('foo.html')});

$location.path('/foo');
$rootScope.$digest();
expect($location.path()).toBe('/foo');
expect($sce.valueOf($route.current.templateUrl)).toBe('foo.html');
});
});


describe('redirection', function() {
it('should support redirection via redirectTo property by updating $location', function() {
module(function($routeProvider) {
Expand Down