From 54b3654fbd2cde5c165d72e2c199d87f0b8a3843 Mon Sep 17 00:00:00 2001 From: Matthew James Davis Date: Wed, 15 Aug 2018 20:30:07 +0900 Subject: [PATCH] fix: Prevent running CanDeactivatePreviousStep twice in same navigation To handle this, I've added a new parameter to the router called `couldDeactivate` which is set to true whenever the CanDeactivatePreviousStep completes and is reset on every navigation. If a redirect is returned from the CanActivateNextStep, then this value will be true and it is used to build a special pipeline that skips the CanDeactivatePreviousStep on the next pipeline run. fixes #45 --- src/activation.js | 4 +++- src/app-router.js | 3 ++- src/pipeline-provider.js | 8 ++++++-- src/router.js | 6 ++++++ test/activation.spec.js | 38 +++++++++++++++++++++----------------- 5 files changed, 38 insertions(+), 21 deletions(-) diff --git a/src/activation.js b/src/activation.js index 0dc1bd42..26cb14d0 100644 --- 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 index 9b2339f4..35ad0bdd 100644 --- 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 index eb5c3eb9..6a1bf904 100644 --- 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 index c29061d1..576080e8 100644 --- 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 index 230ce1ed..1fc37663 100644 --- a/test/activation.spec.js +++ b/test/activation.spec.js @@ -19,34 +19,38 @@ 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); @@ -54,7 +58,7 @@ describe('activation', () => { 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); });