Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
fix(transitionHook): Prevent queued hookFn to be called if deregister…
…ed (#2939)

* fix(transitionHook): Prevent queued hookFn to be called if deregistered

Fix a bug where queued hooks are called even if they are deregistered

* fix(transitionHook): Update tests for API change

Closes #2928
  • Loading branch information
elboman authored and christopherthielen committed Aug 23, 2016
1 parent 696148f commit 39e1ba7
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 8 deletions.
2 changes: 1 addition & 1 deletion src/transition/hookBuilder.ts
Expand Up @@ -98,7 +98,7 @@ export class HookBuilder {
return matchingNodes.map(node => {
let _options = extend({ bind: hook.bind, traceData: { hookType, context: node} }, this.baseHookOptions, options);
let state = _options.stateHook ? node.state : null;
let transitionHook = new TransitionHook(this.transition, state, hook.callback, _options);
let transitionHook = new TransitionHook(this.transition, state, hook, _options);
return <HookTuple> { hook, node, transitionHook };
});
};
Expand Down
3 changes: 3 additions & 0 deletions src/transition/hookRegistry.ts
Expand Up @@ -49,12 +49,14 @@ export class EventHook implements IEventHook {
matchCriteria: HookMatchCriteria;
priority: number;
bind: any;
_deregistered: boolean;

constructor(matchCriteria: HookMatchCriteria, callback: HookFn, options: HookRegOptions = <any>{}) {
this.callback = callback;
this.matchCriteria = extend({ to: true, from: true, exiting: true, retained: true, entering: true }, matchCriteria);
this.priority = options.priority || 0;
this.bind = options.bind || null;
this._deregistered = false;
}

private static _matchingNodes(nodes: PathNode[], criterion: HookMatchCriterion): PathNode[] {
Expand Down Expand Up @@ -99,6 +101,7 @@ function makeHookRegistrationFn(hooks: ITransitionEvents, name: string): IHookRe
hooks[name].push(eventHook);

return function deregisterEventHook() {
eventHook._deregistered = true;
removeFrom(hooks[name])(eventHook);
};
};
Expand Down
1 change: 1 addition & 0 deletions src/transition/interface.ts
Expand Up @@ -762,4 +762,5 @@ export interface IEventHook {
priority?: number;
bind?: any;
matches: (treeChanges: TreeChanges) => IMatchingNodes;
_deregistered: boolean;
}
14 changes: 8 additions & 6 deletions src/transition/transitionHook.ts
@@ -1,5 +1,5 @@
/** @module transition */ /** for typedoc */
import {TransitionHookOptions, HookFn, HookResult} from "./interface";
import {TransitionHookOptions, IEventHook, HookResult} from "./interface";
import {defaults, noop} from "../common/common";
import {fnToString, maxLength} from "../common/strings";
import {isDefined, isPromise } from "../common/predicates";
Expand All @@ -26,21 +26,23 @@ let defaultOptions: TransitionHookOptions = {
export class TransitionHook {
constructor(private transition: Transition,
private stateContext: State,
private hookFn: HookFn,
private eventHook: IEventHook,
private options: TransitionHookOptions) {
this.options = defaults(options, defaultOptions);
}

private isSuperseded = () => this.options.current() !== this.options.transition;

invokeHook(): Promise<HookResult> {
let { options, hookFn } = this;
let { options, eventHook } = this;
trace.traceHookInvocation(this, options);
if (options.rejectIfSuperseded && this.isSuperseded()) {
return Rejection.superseded(options.current()).toPromise();
}

let hookResult = hookFn.call(options.bind, this.transition, this.stateContext);
let hookResult = !eventHook._deregistered
? eventHook.callback.call(options.bind, this.transition, this.stateContext)
: undefined;
return this.handleHookResult(hookResult);
}

Expand Down Expand Up @@ -73,10 +75,10 @@ export class TransitionHook {
}

toString() {
let { options, hookFn } = this;
let { options, eventHook } = this;
let event = parse("traceData.hookType")(options) || "internal",
context = parse("traceData.context.state.name")(options) || parse("traceData.context")(options) || "unknown",
name = fnToString(hookFn);
name = fnToString(eventHook.callback);
return `${event} context: ${context}, ${maxLength(200, name)}`;
}

Expand Down
2 changes: 1 addition & 1 deletion test/core/hookBuilderSpec.ts
Expand Up @@ -62,7 +62,7 @@ describe('HookBuilder:', function() {
expect(typeof callback).toBe('function')
});

const getFn = x => x['hookFn'];
const getFn = x => x['eventHook']['callback'];

describe('HookMatchCriteria', function() {

Expand Down

0 comments on commit 39e1ba7

Please sign in to comment.