Merge pull request #616 from davismj/fix/prevent-repeat-can-deactivate

fix: Prevent running CanDeactivatePreviousStep twice in same navigation
aurelia · Aug 16, 2018 · 2a8a166 · 2a8a166
2 parents 9246613 + 54b3654
commit 2a8a166
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 21 deletions.
diff --git a/src/activation.js b/src/activation.js
@@ -25,7 +25,7 @@ export class ActivateNextStep {
   }
 }
 
-function processDeactivatable(navigationInstruction: NavigationInstruction, callbackName: string, next: Funcion, ignoreResult: boolean) {
+function processDeactivatable(navigationInstruction: NavigationInstruction, callbackName: string, next: Function, ignoreResult: boolean) {
   const plan = navigationInstruction.plan;
   let infos = findDeactivatable(plan, callbackName);
   let i = infos.length; //query from inside out
@@ -49,6 +49,8 @@ function processDeactivatable(navigationInstruction: NavigationInstruction, call
       }
     }
 
+    navigationInstruction.router.couldDeactivate = true;
+
     return next();
   }
 

diff --git a/src/app-router.js b/src/app-router.js
@@ -156,7 +156,7 @@ export class AppRouter extends Router {
         throw new Error('Maximum navigation attempts exceeded. Giving up.');
       }
 
-      let pipeline = this.pipelineProvider.createPipeline();
+      let pipeline = this.pipelineProvider.createPipeline(!this.couldDeactivate);
 
       return pipeline
         .run(instruction)
@@ -230,6 +230,7 @@ function resolveInstruction(instruction, result, isInnerInstruction, router) {
     router.isNavigatingRefresh = false;
     router.isNavigatingForward = false;
     router.isNavigatingBack = false;
+    router.couldDeactivate = false;
 
     let eventName;
 

diff --git a/src/pipeline-provider.js b/src/pipeline-provider.js
@@ -52,9 +52,13 @@ export class PipelineProvider {
   /**
   * Create the navigation pipeline.
   */
-  createPipeline(): Pipeline {
+  createPipeline(useCanDeactivateStep: boolean = true): Pipeline {
     let pipeline = new Pipeline();
-    this.steps.forEach(step => pipeline.addStep(this.container.get(step)));
+    this.steps.forEach(step => {
+      if (useCanDeactivateStep || step !== CanDeactivatePreviousStep) {
+        pipeline.addStep(this.container.get(step));
+      }
+    });
     return pipeline;
   }
 

diff --git a/src/router.js b/src/router.js
@@ -84,6 +84,11 @@ export class Router {
   */
   isNavigatingRefresh: boolean;
 
+  /**
+  * True if the previous instruction successfully completed the CanDeactivatePreviousStep in the current navigation.
+  */
+  couldDeactivate: boolean;
+
   /**
   * The currently active navigation tracker.
   */
@@ -150,6 +155,7 @@ export class Router {
     this.isNavigatingRefresh = false;
     this.isNavigatingForward = false;
     this.isNavigatingBack = false;
+    this.couldDeactivate = false;
     this.navigation = [];
     this.currentInstruction = null;
     this.viewPortDefaults = {};

diff --git a/test/activation.spec.js b/test/activation.spec.js
@@ -19,42 +19,46 @@ describe('activation', () => {
       };
     };
 
+    let instructionFactory = (instruction) => {
+      return Object.assign({ router: {} }, instruction);
+    }
+
     beforeEach(() => {
       step = new CanDeactivatePreviousStep();
       state = createPipelineState();
     });
 
     it('should return true for context that canDeactivate', () => {
-      let instruction = { plan: { first: viewPortFactory(() => (true)) } };
+      let instruction = instructionFactory({ plan: { first: viewPortFactory(() => (true)) } });
 
       step.run(instruction, state.next);
       expect(state.result).toBe(true);
     });
 
     it('should return true for context that canDeactivate with activationStrategy.replace', () => {
-      let instruction = { plan: { first: viewPortFactory(() => (true), activationStrategy.replace) } };
+      let instruction = instructionFactory({ plan: { first: viewPortFactory(() => (true), activationStrategy.replace) } });
 
       step.run(instruction, state.next);
       expect(state.result).toBe(true);
     });
 
     it('should cancel for context that cannot Deactivate', () => {
-      let instruction = { plan: { first: viewPortFactory(() => (false)) } };
+      let instruction = instructionFactory({ plan: { first: viewPortFactory(() => (false)) } });
 
       step.run(instruction, state.next);
       expect(state.rejection).toBeTruthy();
     });
 
     it('should return true for context that cannot Deactivate with unknown strategy', () => {
-      let instruction = { plan: { first: viewPortFactory(() => (false), 'unknown') } };
+      let instruction = instructionFactory({ plan: { first: viewPortFactory(() => (false), 'unknown') } });
 
       step.run(instruction, state.next);
       expect(state.result).toBe(true);
     });
 
 
     it('should return true for context that canDeactivate with a promise', (done) => {
-      let instruction = { plan: {first: viewPortFactory(() => (Promise.resolve(true))) } };
+      let instruction = instructionFactory({ plan: {first: viewPortFactory(() => (Promise.resolve(true))) } });
 
       step.run(instruction, state.next).then(() => {
         expect(state.result).toBe(true);
@@ -63,7 +67,7 @@ describe('activation', () => {
     });
 
     it('should cancel for context that cantDeactivate with a promise', (done) => {
-      let instruction = { plan: {first: viewPortFactory(() => (Promise.resolve(false))) } };
+      let instruction = instructionFactory({ plan: {first: viewPortFactory(() => (Promise.resolve(false))) } });
 
       step.run(instruction, state.next).then(() => {
         expect(state.rejection).toBeTruthy();
@@ -72,7 +76,7 @@ describe('activation', () => {
     });
 
     it('should cancel for context that throws in canDeactivate', (done) => {
-      let instruction = { plan: {first: viewPortFactory(() => { throw new Error('oops'); }) } };
+      let instruction = instructionFactory({ plan: {first: viewPortFactory(() => { throw new Error('oops'); }) } });
 
       step.run(instruction, state.next).then(() => {
         expect(state.rejection).toBeTruthy();
@@ -81,30 +85,30 @@ describe('activation', () => {
     });
 
     it('should return true when all plans return true', () => {
-      let instruction = { plan: { first: viewPortFactory(() => (true)), second: viewPortFactory(() => (true))} };
+      let instruction = instructionFactory({ plan: { first: viewPortFactory(() => (true)), second: viewPortFactory(() => (true))} });
 
       step.run(instruction, state.next);
       expect(state.result).toBe(true);
     });
 
     it('should cancel when some plans return false', () => {
-      let instruction = { plan: {first: viewPortFactory(() => (true)), second: viewPortFactory(() => (false))} };
+      let instruction = instructionFactory({ plan: {first: viewPortFactory(() => (true)), second: viewPortFactory(() => (false))} });
 
       step.run(instruction, state.next);
       expect(state.rejection).toBeTruthy();
     });
 
     it('should pass a navigationInstruction to the callback function', () => {
-      const instruction = { plan: { first: viewPortFactory(() => (true)) } };
+      const instruction = instructionFactory({ plan: { first: viewPortFactory(() => (true)) } });
       const viewModel = instruction.plan.first.prevComponent.viewModel;
       spyOn(viewModel, 'canDeactivate').and.callThrough();
       step.run(instruction, state.next);
-      expect(viewModel.canDeactivate).toHaveBeenCalledWith(instruction);      
+      expect(viewModel.canDeactivate).toHaveBeenCalledWith(instruction);
     });
 
     describe('with a childNavigationInstruction', () => {
         let viewPort = viewPortFactory(() => (true));
-        let instruction = { plan: { first: viewPort } };
+        let instruction = instructionFactory({ plan: { first: viewPort } });
 
       describe('when navigating on the parent', () => {
 
@@ -116,18 +120,18 @@ describe('activation', () => {
 
         it('should return true when the currentInstruction can deactivate', () => {
           let viewPort = viewPortFactory(() => (true), activationStrategy.replace);
-          let currentInstruction = { viewPortInstructions: { first: viewPortInstructionFactory(() => (true)) } };
+          let currentInstruction = instructionFactory({ viewPortInstructions: { first: viewPortInstructionFactory(() => (true)) } });
           viewPort.prevComponent.childRouter = { currentInstruction };
-          let instruction = { plan: { first: viewPort } };
+          let instruction = instructionFactory({ plan: { first: viewPort } });
           step.run(instruction, state.next);
           expect(state.result).toBe(true);
         });
 
         it('should cancel when router instruction cannot deactivate', () => {
           let viewPort = viewPortFactory(() => (true), activationStrategy.replace);
-          let currentInstruction = { viewPortInstructions: { first: viewPortInstructionFactory(() => (false)) } };
+          let currentInstruction = instructionFactory({ viewPortInstructions: { first: viewPortInstructionFactory(() => (false)) } });
           viewPort.prevComponent.childRouter = { currentInstruction };
-          let instruction = { plan: { first: viewPort } };
+          let instruction = instructionFactory({ plan: { first: viewPort } });
           step.run(instruction, state.next);
           expect(state.rejection).toBeTruthy();
         });
@@ -204,7 +208,7 @@ describe('activation', () => {
           done();
         }
       }
-    
+
       instruction.addViewPortInstruction('my-view-port', 'ignored', 'ignored', { viewModel });
       step.run(instruction, state.next);
     });