From 9486590e1be7282bb0e87586a35ca0bee6c64ee0 Mon Sep 17 00:00:00 2001 From: Vojta Jina Date: Sun, 19 Feb 2012 12:31:11 -0800 Subject: 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 --- src/service/route.js | 18 +--- src/widgets.js | 39 +++++--- test/service/routeSpec.js | 233 +++++++++++++--------------------------------- test/widgetsSpec.js | 182 ++++++++++++++++++++++++++++++++++-- 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 += ';'; - } 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();;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, '
partial
', {}]); + $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']); + }); + }); }); -- cgit v1.2.3