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

@@ -237,22 +237,16 @@ function $RouteProvider(){
 
     function updateRoute() {
       var next = parseRoute(),
-          last = $route.current,
+          last = $route.current;
-          Controller;
 
       if (next && last && next.$route === last.$route
           && equals(next.pathParams, last.pathParams) && !next.reloadOnSearch && !forceReload) {
-        next.scope = last.scope;
+        last.params = next.params;
-        $route.current = next;
+        copy(last.params, $routeParams);
-        copy(next.params, $routeParams);
+        $rootScope.$broadcast('$routeUpdate', last);
-        last.scope && last.scope.$emit('$routeUpdate');
+      } else if (next || last) {
-      } else {
         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('');
     }
-
-
   }];
 }

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, previous) {
-      scope.$on('$afterRouteChange', function(event, next) {
         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});
-          }
-        }
-      }
     }
   };
 }];

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) {
+    function NotFoundCtrl() {}
-      $scope.notFoundProp = 'not found!';
-    }
 
     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('/one', {}); // no otherwise defined
-      $routeProvider.when('/bar', {template: 'bar.html', controller: function() {this.name = 'BAR';}});
-      $routeProvider.when('/baz', {template: 'baz.html'});
     });
 
-    inject(function($route, $location, $rootScope) {
+    inject(function($rootScope, $route, $location) {
-      expect($rootScope.$childHead).toEqual(null);
+      $rootScope.$on('$beforeRouteChange', routeChangeSpy);
+      $rootScope.$on('$afterRouteChange', routeChangeSpy);
 
-      $location.path('/foo');
       $rootScope.$digest();
-      expect($rootScope.$$childHead.$id).toBeTruthy();
+      expect(routeChangeSpy).not.toHaveBeenCalled();
-      expect($rootScope.$$childHead.$id).toEqual($rootScope.$$childTail.$id);
 
-      $location.path('/bar');
+      $location.path('/no-route-here');
       $rootScope.$digest();
-      expect($rootScope.$$childHead.$id).toBeTruthy();
+      expect(routeChangeSpy).not.toHaveBeenCalled();
-      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);
-
-      $location.path('/');
-      $rootScope.$digest();
-      expect($rootScope.$$childHead).toEqual(null);
-      expect($rootScope.$$childTail).toEqual(null);
-    });
-  });
-
-
-  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');
-      $rootScope.$digest();
-      expect(injectedRoute).toBe($route);
     });
   });
 
@@ -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'),
+      var routeChange = jasmine.createSpy('route change'),
-          routeUpdateEvent = jasmine.createSpy('route reload');
+          routeUpdate = jasmine.createSpy('route update');
-
-      function FooCtrl($scope) {
-        reloaded();
-        $scope.$on('$routeUpdate', routeUpdateEvent);
-      }
 
       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(routeChange).toHaveBeenCalled();
-        expect(routeUpdateEvent).not.toHaveBeenCalled();
+        expect(routeChange.callCount).toBe(2);
-        reloaded.reset();
+        expect(routeUpdate).not.toHaveBeenCalled();
+        routeChange.reset();
 
         // don't trigger reload
         $location.search({foo: 'bar'});
         $rootScope.$digest();
-        expect(reloaded).not.toHaveBeenCalled();
+        expect(routeChange).not.toHaveBeenCalled();
-        expect(routeUpdateEvent).toHaveBeenCalled();
+        expect(routeUpdate).toHaveBeenCalled();
       });
     });
 
 
     it('should reload reloadOnSearch route when url differs only in route path param', function() {
-      var reloaded = jasmine.createSpy('routeReload'),
+      var routeChange = jasmine.createSpy('route change');
-          onRouteChange = jasmine.createSpy('onRouteChange');
-
-      function FooCtrl() {
-        reloaded();
-      }
 
       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(routeChange).not.toHaveBeenCalled();
-        expect(onRouteChange).not.toHaveBeenCalled();
 
         $location.path('/foo/aaa');
         $rootScope.$digest();
-        expect(reloaded).toHaveBeenCalled();
+        expect(routeChange).toHaveBeenCalled();
-        expect(onRouteChange).toHaveBeenCalled();
+        expect(routeChange.callCount).toBe(2);
-        reloaded.reset();
+        routeChange.reset();
-        onRouteChange.reset();
 
         $location.path('/foo/bbb');
         $rootScope.$digest();
-        expect(reloaded).toHaveBeenCalled();
+        expect(routeChange).toHaveBeenCalled();
-        expect(onRouteChange).toHaveBeenCalled();
+        expect(routeChange.callCount).toBe(2);
-        reloaded.reset();
+        routeChange.reset();
-        onRouteChange.reset();
 
         $location.search({foo: 'bar'});
         $rootScope.$digest();
-        expect(reloaded).not.toHaveBeenCalled();
+        expect(routeChange).not.toHaveBeenCalled();
-        expect(onRouteChange).not.toHaveBeenCalled();
       });
     });
 
 
     it('should update params when reloadOnSearch is disabled and .search() changes', function() {
-      var routeParams = jasmine.createSpy('routeParams');
+      var routeParamsWatcher = jasmine.createSpy('routeParamsWatcher');
-
-      function FooCtrl($scope, $route) {
-        $scope.$watch(function() {
-          return $route.current.params;
-        }, function(value) {
-          routeParams(value);
-        });
-      }
 
       module(function($routeProvider) {
-        $routeProvider.when('/foo', {controller: FooCtrl});
+        $routeProvider.when('/foo', {controller: noop});
-        $routeProvider.when('/bar/:barId', {controller: FooCtrl, reloadOnSearch: false});
+        $routeProvider.when('/bar/:barId', {controller: noop, reloadOnSearch: false});
       });
 
-      inject(function($route, $location, $rootScope) {
+      inject(function($route, $location, $rootScope, $routeParams) {
-        expect(routeParams).not.toHaveBeenCalled();
+        $rootScope.$watch(function() {
+          return $routeParams;
+        }, function(value) {
+          routeParamsWatcher(value);
+        }, true);
+
+        expect(routeParamsWatcher).not.toHaveBeenCalled();
 
         $location.path('/foo');
         $rootScope.$digest();
-        expect(routeParams).toHaveBeenCalledWith({});
+        expect(routeParamsWatcher).toHaveBeenCalledWith({});
-        routeParams.reset();
+        routeParamsWatcher.reset();
 
         // trigger reload
         $location.search({foo: 'bar'});
         $rootScope.$digest();
-        expect(routeParams).toHaveBeenCalledWith({foo: 'bar'});
+        expect(routeParamsWatcher).toHaveBeenCalledWith({foo: 'bar'});
-        routeParams.reset();
+        routeParamsWatcher.reset();
 
         $location.path('/bar/123').search({});
         $rootScope.$digest();
-        expect(routeParams).toHaveBeenCalledWith({barId: '123'});
+        expect(routeParamsWatcher).toHaveBeenCalledWith({barId: '123'});
-        routeParams.reset();
+        routeParamsWatcher.reset();
 
         // don't trigger reload
         $location.search({foo: 'bar'});
         $rootScope.$digest();
-        expect(routeParams).toHaveBeenCalledWith({barId: '123', foo: 'bar'});
+        expect(routeParamsWatcher).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']);
       });
     });
 
@@ -514,29 +410,30 @@ describe('$route', function() {
     describe('reload', function() {
 
       it('should reload even if reloadOnSearch is false', function() {
-        var count = 0;
+        var routeChangeSpy = jasmine.createSpy('route change');
-
-        function FooCtrl() { count ++; }
 
         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();
         });
       });
     });

test/widgetsSpec.js

@@ -633,16 +633,35 @@ describe('widget', function() {
     }));
 
 
-    it('should create controller instance on $afterRouteChange event', inject(
+    it('should instantiate controller after compiling the content', function() {
-      function($route, $rootScope) {
+      var log = [], controllerScope,
-        var controllerScope;
+          Ctrl = function($scope) {
-        $route.current = { controller: function($scope) { controllerScope = $scope; } };
+            controllerScope = $scope;
-        $rootScope.$broadcast('$afterRouteChange', $route.current);
+            log.push('ctrl-init');
+          };
 
-        expect(controllerScope.$parent.$id).toBe($rootScope.$id);
+      module(function($compileProvider, $routeProvider) {
-        expect(controllerScope.$id).toBe($route.current.scope.$id);
+        $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']);
+      });
+    });
   });