fix(router): RouterOutlet loads component twice in a race condition

mhevery · mhevery · commit 2f581ffc8846 · 2016-03-16T22:34:54.000Z
Closes #7497 Closes #7545
diff --git a/modules/angular2/src/core/linker/dynamic_component_loader.ts b/modules/angular2/src/core/linker/dynamic_component_loader.ts
@@ -59,7 +59,7 @@ export abstract class ComponentRef {
    *
    * TODO(i): rename to destroy to be consistent with AppViewManager and ViewContainerRef
    */
-  abstract dispose();
+  abstract dispose(): void;
 }
 
 export class ComponentRef_ extends ComponentRef {
@@ -84,7 +84,7 @@ export class ComponentRef_ extends ComponentRef {
    */
   get hostComponentType(): Type { return this.componentType; }
 
-  dispose() { this._dispose(); }
+  dispose(): void { this._dispose(); }
 }
 
 /**
diff --git a/modules/angular2/src/router/directives/router_outlet.ts b/modules/angular2/src/router/directives/router_outlet.ts
@@ -34,7 +34,7 @@ let _resolveToTrue = PromiseWrapper.resolve(true);
 @Directive({selector: 'router-outlet'})
 export class RouterOutlet implements OnDestroy {
   name: string = null;
-  private _componentRef: ComponentRef = null;
+  private _componentRef: Promise<ComponentRef> = null;
   private _currentInstruction: ComponentInstruction = null;
 
   constructor(private _elementRef: ElementRef, private _loader: DynamicComponentLoader,
@@ -62,14 +62,17 @@ export class RouterOutlet implements OnDestroy {
       provide(RouteParams, {useValue: new RouteParams(nextInstruction.params)}),
       provide(routerMod.Router, {useValue: childRouter})
     ]);
-    return this._loader.loadNextToLocation(componentType, this._elementRef, providers)
-        .then((componentRef) => {
-          this._componentRef = componentRef;
-          if (hasLifecycleHook(hookMod.routerOnActivate, componentType)) {
-            return (<OnActivate>this._componentRef.instance)
-                .routerOnActivate(nextInstruction, previousInstruction);
-          }
-        });
+    this._componentRef =
+        this._loader.loadNextToLocation(componentType, this._elementRef, providers);
+    return this._componentRef.then((componentRef) => {
+      if (hasLifecycleHook(hookMod.routerOnActivate, componentType)) {
+        return this._componentRef.then(
+            (ref: ComponentRef) =>
+                (<OnActivate>ref.instance).routerOnActivate(nextInstruction, previousInstruction));
+      } else {
+        return componentRef;
+      }
+    });
   }
 
   /**
@@ -86,12 +89,14 @@ export class RouterOutlet implements OnDestroy {
     // a new one.
     if (isBlank(this._componentRef)) {
       return this.activate(nextInstruction);
+    } else {
+      return PromiseWrapper.resolve(
+          hasLifecycleHook(hookMod.routerOnReuse, this._currentInstruction.componentType) ?
+              this._componentRef.then(
+                  (ref: ComponentRef) =>
+                      (<OnReuse>ref.instance).routerOnReuse(nextInstruction, previousInstruction)) :
+              true);
     }
-    return PromiseWrapper.resolve(
-        hasLifecycleHook(hookMod.routerOnReuse, this._currentInstruction.componentType) ?
-            (<OnReuse>this._componentRef.instance)
-                .routerOnReuse(nextInstruction, previousInstruction) :
-            true);
   }
 
   /**
@@ -102,14 +107,16 @@ export class RouterOutlet implements OnDestroy {
     var next = _resolveToTrue;
     if (isPresent(this._componentRef) && isPresent(this._currentInstruction) &&
         hasLifecycleHook(hookMod.routerOnDeactivate, this._currentInstruction.componentType)) {
-      next = <Promise<boolean>>PromiseWrapper.resolve(
-          (<OnDeactivate>this._componentRef.instance)
-              .routerOnDeactivate(nextInstruction, this._currentInstruction));
+      next = this._componentRef.then(
+          (ref: ComponentRef) =>
+              (<OnDeactivate>ref.instance)
+                  .routerOnDeactivate(nextInstruction, this._currentInstruction));
     }
     return next.then((_) => {
       if (isPresent(this._componentRef)) {
-        this._componentRef.dispose();
+        var onDispose = this._componentRef.then((ref: ComponentRef) => ref.dispose());
         this._componentRef = null;
+        return onDispose;
       }
     });
   }
@@ -127,11 +134,13 @@ export class RouterOutlet implements OnDestroy {
       return _resolveToTrue;
     }
     if (hasLifecycleHook(hookMod.routerCanDeactivate, this._currentInstruction.componentType)) {
-      return <Promise<boolean>>PromiseWrapper.resolve(
-          (<CanDeactivate>this._componentRef.instance)
-              .routerCanDeactivate(nextInstruction, this._currentInstruction));
+      return this._componentRef.then(
+          (ref: ComponentRef) =>
+              (<CanDeactivate>ref.instance)
+                  .routerCanDeactivate(nextInstruction, this._currentInstruction));
+    } else {
+      return _resolveToTrue;
     }
-    return _resolveToTrue;
   }
 
   /**
@@ -151,8 +160,9 @@ export class RouterOutlet implements OnDestroy {
         this._currentInstruction.componentType != nextInstruction.componentType) {
       result = false;
     } else if (hasLifecycleHook(hookMod.routerCanReuse, this._currentInstruction.componentType)) {
-      result = (<CanReuse>this._componentRef.instance)
-                   .routerCanReuse(nextInstruction, this._currentInstruction);
+      result = this._componentRef.then(
+          (ref: ComponentRef) =>
+              (<CanReuse>ref.instance).routerCanReuse(nextInstruction, this._currentInstruction));
     } else {
       result = nextInstruction == this._currentInstruction ||
                (isPresent(nextInstruction.params) && isPresent(this._currentInstruction.params) &&
diff --git a/tools/public_api_guard/public_api_spec.ts b/tools/public_api_guard/public_api_spec.ts
@@ -106,7 +106,7 @@ const CORE = [
   'ComponentMetadata.viewProviders:any[]',
   'ComponentRef',
   'ComponentRef.componentType:Type',
-  'ComponentRef.dispose():any',
+  'ComponentRef.dispose():void',
   'ComponentRef.hostComponent:any',
   'ComponentRef.hostView:HostViewRef',
   'ComponentRef.injector:Injector',

Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ export abstract class ComponentRef {`
`59`	`59`	`*`
`60`	`60`	`* TODO(i): rename to destroy to be consistent with AppViewManager and ViewContainerRef`
`61`	`61`	`*/`
`62`		`- abstract dispose();`
	`62`	`+ abstract dispose(): void;`
`63`	`63`	`}`
`64`	`64`
`65`	`65`	`export class ComponentRef_ extends ComponentRef {`
`@@ -84,7 +84,7 @@ export class ComponentRef_ extends ComponentRef {`
`84`	`84`	`*/`
`85`	`85`	`get hostComponentType(): Type { return this.componentType; }`
`86`	`86`
`87`		`- dispose() { this._dispose(); }`
	`87`	`+ dispose(): void { this._dispose(); }`
`88`	`88`	`}`
`89`	`89`
`90`	`90`	`/**`