aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorVojta Jina2012-02-19 12:31:11 -0800
committerVojta Jina2012-02-28 17:46:58 -0800
commit9486590e1be7282bb0e87586a35ca0bee6c64ee0 (patch)
treebb7a808e077f134ddc26b84e35a406e62dbf117d
parente31d1c287d972d633bdaf9c385d3012192f64918 (diff)
downloadangular.js-9486590e1be7282bb0e87586a35ca0bee6c64ee0.tar.bz2
refactor(ng:view) Make $route scope agnostic, add $contentLoaded event
Problems: - controller was instantiated immediately on $afterRouteChange (even if no content), that's different compare to ng:controller, which instantiates controllers after compiling - route listened on current scope ($afterRouteChange), so if you were listening on $rootScope ($afterRouteChange), you get called first and current.scope === undefined, which is flaky - route handles scope destroying, but scope is created by ng:view - route fires after/before route change even if there is no route (when no otherwise specified) Solution: - route has no idea about scope, whole scope business moved to ng:view (creating/destroying) - scope is created (and controller instantiated) AFTER compiling the content - that means on $afterRouteChange - there is no scope yet (current.scope === undefined) - added $contentLoaded event fired by ng:view, after linking the content
-rw-r--r--src/service/route.js18
-rw-r--r--src/widgets.js39
-rw-r--r--test/service/routeSpec.js233
-rw-r--r--test/widgetsSpec.js182
4 files changed, 267 insertions, 205 deletions
diff --git a/src/service/route.js b/src/service/route.js
index b7f8bd02..8d8086af 100644
--- a/src/service/route.js
+++ b/src/service/route.js
@@ -237,22 +237,16 @@ function $RouteProvider(){
function updateRoute() {
var next = parseRoute(),
- last = $route.current,
- Controller;
+ last = $route.current;
if (next && last && next.$route === last.$route
&& equals(next.pathParams, last.pathParams) && !next.reloadOnSearch && !forceReload) {
- next.scope = last.scope;
- $route.current = next;
- copy(next.params, $routeParams);
- last.scope && last.scope.$emit('$routeUpdate');
- } else {
+ last.params = next.params;
+ copy(last.params, $routeParams);
+ $rootScope.$broadcast('$routeUpdate', last);
+ } else if (next || last) {
forceReload = false;
$rootScope.$broadcast('$beforeRouteChange', next, last);
- if (last && last.scope) {
- last.scope.$destroy();
- last.scope = null;
- }
$route.current = next;
if (next) {
if (next.redirectTo) {
@@ -308,7 +302,5 @@ function $RouteProvider(){
});
return result.join('');
}
-
-
}];
}
diff --git a/src/widgets.js b/src/widgets.js
index bc41d761..6d64730f 100644
--- a/src/widgets.js
+++ b/src/widgets.js
@@ -544,22 +544,29 @@ var ngViewDirective = ['$http', '$templateCache', '$route', '$anchorScroll', '$c
return {
terminal: true,
link: function(scope, element) {
- var changeCounter = 0;
+ var changeCounter = 0,
+ lastScope;
- processRoute($route.current);
- scope.$on('$afterRouteChange', function(event, next) {
+ scope.$on('$afterRouteChange', function(event, next, previous) {
changeCounter++;
- processRoute(next);
});
scope.$watch(function() {return changeCounter;}, function(newChangeCounter) {
var template = $route.current && $route.current.template;
+ function destroyLastScope() {
+ if (lastScope) {
+ lastScope.$destroy();
+ lastScope = null;
+ }
+ }
+
function clearContent() {
// ignore callback if another route change occured since
if (newChangeCounter == changeCounter) {
element.html('');
}
+ destroyLastScope();
}
if (template) {
@@ -567,7 +574,20 @@ var ngViewDirective = ['$http', '$templateCache', '$route', '$anchorScroll', '$c
// ignore callback if another route change occured since
if (newChangeCounter == changeCounter) {
element.html(response);
- $compile(element.contents())($route.current.scope);
+ destroyLastScope();
+
+ var link = $compile(element.contents()),
+ current = $route.current;
+
+ lastScope = current.scope = scope.$new();
+ if (current.controller) {
+ $controller(current.controller, {$scope: lastScope});
+ }
+
+ link(lastScope);
+ lastScope.$emit('$contentLoaded');
+
+ // $anchorScroll might listen on event...
$anchorScroll();
}
}).error(clearContent);
@@ -575,15 +595,6 @@ var ngViewDirective = ['$http', '$templateCache', '$route', '$anchorScroll', '$c
clearContent();
}
});
-
- function processRoute(route) {
- if (route) {
- route.scope = scope.$new();
- if (route.controller) {
- $controller(route.controller, {$scope: route.scope});
- }
- }
- }
}
};
}];
diff --git a/test/service/routeSpec.js b/test/service/routeSpec.js
index 97532f38..6b8127a0 100644
--- a/test/service/routeSpec.js
+++ b/test/service/routeSpec.js
@@ -2,30 +2,14 @@
describe('$route', function() {
- beforeEach(module(function() {
- return function($rootScope, $controller) {
- $rootScope.$on('$afterRouteChange', function(event, next) {
- // emulate ng:view scope creation
- if (next) {
- next.scope = $rootScope.$new();
- next.controller && $controller(next.controller, {$scope: next.scope});
- }
- });
- };
- }));
-
-
it('should route and fire change event', function() {
var log = '',
lastRoute,
nextRoute;
- function BookChapter() {
- log += '<init>;';
- }
module(function($routeProvider) {
$routeProvider.when('/Book/:book/Chapter/:chapter',
- {controller: BookChapter, template: 'Chapter.html'});
+ {controller: noop, template: 'Chapter.html'});
$routeProvider.when('/Blank');
});
inject(function($route, $location, $rootScope) {
@@ -44,16 +28,14 @@ describe('$route', function() {
$location.path('/Book/Moby/Chapter/Intro').search('p=123');
$rootScope.$digest();
- expect(log).toEqual('before();<init>;after();');
+ expect(log).toEqual('before();after();');
expect($route.current.params).toEqual({book:'Moby', chapter:'Intro', p:'123'});
- var lastId = $route.current.scope.$id;
log = '';
$location.path('/Blank').search('ignore');
$rootScope.$digest();
expect(log).toEqual('before();after();');
expect($route.current.params).toEqual({ignore:true});
- expect($route.current.scope.$id).not.toEqual(lastId);
log = '';
$location.path('/NONE');
@@ -133,9 +115,7 @@ describe('$route', function() {
it('should handle unknown routes with "otherwise" route definition', function() {
- function NotFoundCtrl($scope) {
- $scope.notFoundProp = 'not found!';
- }
+ function NotFoundCtrl() {}
module(function($routeProvider){
$routeProvider.when('/foo', {template: 'foo.html'});
@@ -154,7 +134,6 @@ describe('$route', function() {
expect($route.current.template).toBe('404.html');
expect($route.current.controller).toBe(NotFoundCtrl);
- expect($route.current.scope.notFoundProp).toBe('not found!');
expect(onChangeSpy).toHaveBeenCalled();
onChangeSpy.reset();
@@ -163,55 +142,28 @@ describe('$route', function() {
expect($route.current.template).toEqual('foo.html');
expect($route.current.controller).toBeUndefined();
- expect($route.current.scope.notFoundProp).toBeUndefined();
expect(onChangeSpy).toHaveBeenCalled();
});
});
- it('should $destroy old routes', function() {
+ it('should not fire $after/beforeRouteChange during bootstrap (if no route)', function() {
+ var routeChangeSpy = jasmine.createSpy('route change');
+
module(function($routeProvider) {
- $routeProvider.when('/foo', {template: 'foo.html', controller: function() {this.name = 'FOO';}});
- $routeProvider.when('/bar', {template: 'bar.html', controller: function() {this.name = 'BAR';}});
- $routeProvider.when('/baz', {template: 'baz.html'});
+ $routeProvider.when('/one', {}); // no otherwise defined
});
- inject(function($route, $location, $rootScope) {
- expect($rootScope.$childHead).toEqual(null);
-
- $location.path('/foo');
- $rootScope.$digest();
- expect($rootScope.$$childHead.$id).toBeTruthy();
- expect($rootScope.$$childHead.$id).toEqual($rootScope.$$childTail.$id);
-
- $location.path('/bar');
- $rootScope.$digest();
- expect($rootScope.$$childHead.$id).toBeTruthy();
- expect($rootScope.$$childHead.$id).toEqual($rootScope.$$childTail.$id);
-
- $location.path('/baz');
- $rootScope.$digest();
- expect($rootScope.$$childHead.$id).toBeTruthy();
- expect($rootScope.$$childHead.$id).toEqual($rootScope.$$childTail.$id);
+ inject(function($rootScope, $route, $location) {
+ $rootScope.$on('$beforeRouteChange', routeChangeSpy);
+ $rootScope.$on('$afterRouteChange', routeChangeSpy);
- $location.path('/');
$rootScope.$digest();
- expect($rootScope.$$childHead).toEqual(null);
- expect($rootScope.$$childTail).toEqual(null);
- });
- });
-
+ expect(routeChangeSpy).not.toHaveBeenCalled();
- it('should infer arguments in injection', function() {
- var injectedRoute;
- module(function($routeProvider) {
- $routeProvider.when('/test', {controller: function($route) {injectedRoute = $route;}});
- });
-
- inject(function($route, $location, $rootScope) {
- $location.path('/test');
+ $location.path('/no-route-here');
$rootScope.$digest();
- expect(injectedRoute).toBe($route);
+ expect(routeChangeSpy).not.toHaveBeenCalled();
});
});
@@ -330,12 +282,8 @@ describe('$route', function() {
it('should reload a route when reloadOnSearch is enabled and .search() changes', function() {
var reloaded = jasmine.createSpy('route reload');
- function FooCtrl() {
- reloaded();
- }
-
module(function($routeProvider) {
- $routeProvider.when('/foo', {controller: FooCtrl});
+ $routeProvider.when('/foo', {controller: noop});
});
inject(function($route, $location, $rootScope, $routeParams) {
@@ -356,157 +304,105 @@ describe('$route', function() {
it('should not reload a route when reloadOnSearch is disabled and only .search() changes', function() {
- var reloaded = jasmine.createSpy('route reload'),
- routeUpdateEvent = jasmine.createSpy('route reload');
-
- function FooCtrl($scope) {
- reloaded();
- $scope.$on('$routeUpdate', routeUpdateEvent);
- }
+ var routeChange = jasmine.createSpy('route change'),
+ routeUpdate = jasmine.createSpy('route update');
module(function($routeProvider) {
- $routeProvider.when('/foo', {controller: FooCtrl, reloadOnSearch: false});
+ $routeProvider.when('/foo', {controller: noop, reloadOnSearch: false});
});
inject(function($route, $location, $rootScope) {
- $rootScope.$on('$beforeRouteChange', reloaded);
+ $rootScope.$on('$beforeRouteChange', routeChange);
+ $rootScope.$on('$afterRouteChange', routeChange);
+ $rootScope.$on('$routeUpdate', routeUpdate);
- expect(reloaded).not.toHaveBeenCalled();
+ expect(routeChange).not.toHaveBeenCalled();
$location.path('/foo');
$rootScope.$digest();
- expect(reloaded).toHaveBeenCalled();
- expect(routeUpdateEvent).not.toHaveBeenCalled();
- reloaded.reset();
+ expect(routeChange).toHaveBeenCalled();
+ expect(routeChange.callCount).toBe(2);
+ expect(routeUpdate).not.toHaveBeenCalled();
+ routeChange.reset();
// don't trigger reload
$location.search({foo: 'bar'});
$rootScope.$digest();
- expect(reloaded).not.toHaveBeenCalled();
- expect(routeUpdateEvent).toHaveBeenCalled();
+ expect(routeChange).not.toHaveBeenCalled();
+ expect(routeUpdate).toHaveBeenCalled();
});
});
it('should reload reloadOnSearch route when url differs only in route path param', function() {
- var reloaded = jasmine.createSpy('routeReload'),
- onRouteChange = jasmine.createSpy('onRouteChange');
-
- function FooCtrl() {
- reloaded();
- }
+ var routeChange = jasmine.createSpy('route change');
module(function($routeProvider) {
- $routeProvider.when('/foo/:fooId', {controller: FooCtrl, reloadOnSearch: false});
+ $routeProvider.when('/foo/:fooId', {controller: noop, reloadOnSearch: false});
});
inject(function($route, $location, $rootScope) {
- $rootScope.$on('$beforeRouteChange', onRouteChange);
+ $rootScope.$on('$beforeRouteChange', routeChange);
+ $rootScope.$on('$afterRouteChange', routeChange);
- expect(reloaded).not.toHaveBeenCalled();
- expect(onRouteChange).not.toHaveBeenCalled();
+ expect(routeChange).not.toHaveBeenCalled();
$location.path('/foo/aaa');
$rootScope.$digest();
- expect(reloaded).toHaveBeenCalled();
- expect(onRouteChange).toHaveBeenCalled();
- reloaded.reset();
- onRouteChange.reset();
+ expect(routeChange).toHaveBeenCalled();
+ expect(routeChange.callCount).toBe(2);
+ routeChange.reset();
$location.path('/foo/bbb');
$rootScope.$digest();
- expect(reloaded).toHaveBeenCalled();
- expect(onRouteChange).toHaveBeenCalled();
- reloaded.reset();
- onRouteChange.reset();
+ expect(routeChange).toHaveBeenCalled();
+ expect(routeChange.callCount).toBe(2);
+ routeChange.reset();
$location.search({foo: 'bar'});
$rootScope.$digest();
- expect(reloaded).not.toHaveBeenCalled();
- expect(onRouteChange).not.toHaveBeenCalled();
+ expect(routeChange).not.toHaveBeenCalled();
});
});
it('should update params when reloadOnSearch is disabled and .search() changes', function() {
- var routeParams = jasmine.createSpy('routeParams');
-
- function FooCtrl($scope, $route) {
- $scope.$watch(function() {
- return $route.current.params;
- }, function(value) {
- routeParams(value);
- });
- }
+ var routeParamsWatcher = jasmine.createSpy('routeParamsWatcher');
module(function($routeProvider) {
- $routeProvider.when('/foo', {controller: FooCtrl});
- $routeProvider.when('/bar/:barId', {controller: FooCtrl, reloadOnSearch: false});
+ $routeProvider.when('/foo', {controller: noop});
+ $routeProvider.when('/bar/:barId', {controller: noop, reloadOnSearch: false});
});
- inject(function($route, $location, $rootScope) {
- expect(routeParams).not.toHaveBeenCalled();
+ inject(function($route, $location, $rootScope, $routeParams) {
+ $rootScope.$watch(function() {
+ return $routeParams;
+ }, function(value) {
+ routeParamsWatcher(value);
+ }, true);
+
+ expect(routeParamsWatcher).not.toHaveBeenCalled();
$location.path('/foo');
$rootScope.$digest();
- expect(routeParams).toHaveBeenCalledWith({});
- routeParams.reset();
+ expect(routeParamsWatcher).toHaveBeenCalledWith({});
+ routeParamsWatcher.reset();
// trigger reload
$location.search({foo: 'bar'});
$rootScope.$digest();
- expect(routeParams).toHaveBeenCalledWith({foo: 'bar'});
- routeParams.reset();
+ expect(routeParamsWatcher).toHaveBeenCalledWith({foo: 'bar'});
+ routeParamsWatcher.reset();
$location.path('/bar/123').search({});
$rootScope.$digest();
- expect(routeParams).toHaveBeenCalledWith({barId: '123'});
- routeParams.reset();
+ expect(routeParamsWatcher).toHaveBeenCalledWith({barId: '123'});
+ routeParamsWatcher.reset();
// don't trigger reload
$location.search({foo: 'bar'});
$rootScope.$digest();
- expect(routeParams).toHaveBeenCalledWith({barId: '123', foo: 'bar'});
- });
- });
-
-
- it('should $destroy scope after update and reload', function() {
- // this is a regression of bug, where $route doesn't copy scope when only updating
-
- var log = [];
-
- function logger(msg) {
- return function() {
- log.push(msg);
- };
- }
-
- function createController(name) {
- return function($scope) {
- log.push('init-' + name);
- $scope.$on('$destroy', logger('destroy-' + name));
- $scope.$on('$routeUpdate', logger('route-update'));
- };
- }
-
- module(function($routeProvider) {
- $routeProvider.when('/foo', {controller: createController('foo'), reloadOnSearch: false});
- $routeProvider.when('/bar', {controller: createController('bar')});
- });
-
- inject(function($route, $location, $rootScope) {
- $location.url('/foo');
- $rootScope.$digest();
- expect(log).toEqual(['init-foo']);
-
- $location.search({q: 'some'});
- $rootScope.$digest();
- expect(log).toEqual(['init-foo', 'route-update']);
-
- $location.url('/bar');
- $rootScope.$digest();
- expect(log).toEqual(['init-foo', 'route-update', 'destroy-foo', 'init-bar']);
+ expect(routeParamsWatcher).toHaveBeenCalledWith({barId: '123', foo: 'bar'});
});
});
@@ -514,29 +410,30 @@ describe('$route', function() {
describe('reload', function() {
it('should reload even if reloadOnSearch is false', function() {
- var count = 0;
-
- function FooCtrl() { count ++; }
+ var routeChangeSpy = jasmine.createSpy('route change');
module(function($routeProvider) {
- $routeProvider.when('/bar/:barId', {controller: FooCtrl, reloadOnSearch: false});
+ $routeProvider.when('/bar/:barId', {controller: noop, reloadOnSearch: false});
});
inject(function($route, $location, $rootScope, $routeParams) {
+ $rootScope.$on('$afterRouteChange', routeChangeSpy);
+
$location.path('/bar/123');
$rootScope.$digest();
expect($routeParams).toEqual({barId:'123'});
- expect(count).toEqual(1);
+ expect(routeChangeSpy).toHaveBeenCalledOnce();
+ routeChangeSpy.reset();
$location.path('/bar/123').search('a=b');
$rootScope.$digest();
expect($routeParams).toEqual({barId:'123', a:'b'});
- expect(count).toEqual(1);
+ expect(routeChangeSpy).not.toHaveBeenCalled();
$route.reload();
$rootScope.$digest();
expect($routeParams).toEqual({barId:'123', a:'b'});
- expect(count).toEqual(2);
+ expect(routeChangeSpy).toHaveBeenCalledOnce();
});
});
});
diff --git a/test/widgetsSpec.js b/test/widgetsSpec.js
index 3b245d11..d9517866 100644
--- a/test/widgetsSpec.js
+++ b/test/widgetsSpec.js
@@ -633,16 +633,35 @@ describe('widget', function() {
}));
- it('should create controller instance on $afterRouteChange event', inject(
- function($route, $rootScope) {
- var controllerScope;
- $route.current = { controller: function($scope) { controllerScope = $scope; } };
- $rootScope.$broadcast('$afterRouteChange', $route.current);
-
- expect(controllerScope.$parent.$id).toBe($rootScope.$id);
- expect(controllerScope.$id).toBe($route.current.scope.$id);
- }
- ));
+ it('should instantiate controller after compiling the content', function() {
+ var log = [], controllerScope,
+ Ctrl = function($scope) {
+ controllerScope = $scope;
+ log.push('ctrl-init');
+ };
+
+ module(function($compileProvider, $routeProvider) {
+ $compileProvider.directive('compileLog', function() {
+ return {
+ compile: function() {
+ log.push('compile');
+ }
+ };
+ });
+
+ $routeProvider.when('/some', {template: '/tpl.html', controller: Ctrl});
+ });
+
+ inject(function($route, $rootScope, $templateCache, $location) {
+ $templateCache.put('/tpl.html', [200, '<div compile-log>partial</div>', {}]);
+ $location.path('/some');
+ $rootScope.$digest();
+
+ expect(controllerScope.$parent).toBe($rootScope);
+ expect(controllerScope).toBe($route.current.scope);
+ expect(log).toEqual(['compile', 'ctrl-init']);
+ });
+ });
it('should load content via xhr when route changes', function() {
@@ -842,6 +861,149 @@ describe('widget', function() {
expect(element.text()).toBe('my partial');
});
});
+
+ it('should fire $contentLoaded event when content compiled and linked', function() {
+ var log = [];
+ var logger = function(name) {
+ return function() {
+ log.push(name);
+ };
+ };
+ var Ctrl = function($scope) {
+ $scope.value = 'bound-value';
+ log.push('init-ctrl');
+ };
+
+ module(function($routeProvider) {
+ $routeProvider.when('/foo', {template: 'tpl.html', controller: Ctrl});
+ });
+
+ inject(function($templateCache, $rootScope, $location) {
+ $rootScope.$on('$beforeRouteChange', logger('$beforeRouteChange'));
+ $rootScope.$on('$afterRouteChange', logger('$afterRouteChange'));
+ $rootScope.$on('$contentLoaded', logger('$contentLoaded'));
+
+ $templateCache.put('tpl.html', [200, '{{value}}', {}]);
+ $location.path('/foo');
+ $rootScope.$digest();
+
+ expect(element.text()).toBe('bound-value');
+ expect(log).toEqual(['$beforeRouteChange', '$afterRouteChange', 'init-ctrl',
+ '$contentLoaded']);
+ });
+ });
+
+ it('should destroy previous scope', function() {
+ module(function($routeProvider) {
+ $routeProvider.when('/foo', {template: 'tpl.html'});
+ });
+
+ inject(function($templateCache, $rootScope, $location) {
+ $templateCache.put('tpl.html', [200, 'partial', {}]);
+
+ expect($rootScope.$$childHead).toBeNull();
+ expect($rootScope.$$childTail).toBeNull();
+
+ $location.path('/foo');
+ $rootScope.$digest();
+
+ expect(element.text()).toBe('partial');
+ expect($rootScope.$$childHead).not.toBeNull();
+ expect($rootScope.$$childTail).not.toBeNull();
+
+ $location.path('/non/existing/route');
+ $rootScope.$digest();
+
+ expect(element.text()).toBe('');
+ expect($rootScope.$$childHead).toBeNull();
+ expect($rootScope.$$childTail).toBeNull();
+ });
+ });
+
+
+ it('should destroy previous scope if multiple route changes occur before server responds',
+ function() {
+ var log = [];
+ var createCtrl = function(name) {
+ return function($scope) {
+ log.push('init-' + name);
+ $scope.$on('$destroy', function() {
+ log.push('destroy-' + name);
+ });
+ };
+ };
+
+ module(function($routeProvider) {
+ $routeProvider.when('/one', {template: 'one.html', controller: createCtrl('ctrl1')});
+ $routeProvider.when('/two', {template: 'two.html', controller: createCtrl('ctrl2')});
+ });
+
+ inject(function($httpBackend, $rootScope, $location) {
+ $httpBackend.whenGET('one.html').respond('content 1');
+ $httpBackend.whenGET('two.html').respond('content 2');
+
+ $location.path('/one');
+ $rootScope.$digest();
+ $location.path('/two');
+ $rootScope.$digest();
+
+ $httpBackend.flush();
+ expect(element.text()).toBe('content 2');
+ expect(log).toEqual(['init-ctrl2']);
+
+ $location.path('/non-existing');
+ $rootScope.$digest();
+
+ expect(element.text()).toBe('');
+ expect(log).toEqual(['init-ctrl2', 'destroy-ctrl2']);
+
+ expect($rootScope.$$childHead).toBeNull();
+ expect($rootScope.$$childTail).toBeNull();
+ });
+ });
+
+
+ it('should $destroy scope after update and reload', function() {
+ // this is a regression of bug, where $route doesn't copy scope when only updating
+
+ var log = [];
+
+ function logger(msg) {
+ return function() {
+ log.push(msg);
+ };
+ }
+
+ function createController(name) {
+ return function($scope) {
+ log.push('init-' + name);
+ $scope.$on('$destroy', logger('destroy-' + name));
+ $scope.$on('$routeUpdate', logger('route-update'));
+ };
+ }
+
+ module(function($routeProvider) {
+ $routeProvider.when('/bar', {template: 'tpl.html', controller: createController('bar')});
+ $routeProvider.when('/foo', {
+ template: 'tpl.html', controller: createController('foo'), reloadOnSearch: false});
+ });
+
+ inject(function($templateCache, $location, $rootScope) {
+ $templateCache.put('tpl.html', [200, 'partial', {}]);
+
+ $location.url('/foo');
+ $rootScope.$digest();
+ expect(log).toEqual(['init-foo']);
+
+ $location.search({q: 'some'});
+ $rootScope.$digest();
+ expect(log).toEqual(['init-foo', 'route-update']);
+
+ $location.url('/bar');
+ $rootScope.$digest();
+ expect(log).toEqual(['init-foo', 'route-update', 'destroy-foo', 'init-bar']);
+ });
+ });
});