Skip to content

Commit

Permalink
Simplify TransitionState resolution system.
Browse files Browse the repository at this point in the history
  • Loading branch information
rwjblue committed Sep 10, 2020
1 parent bb16c56 commit e8bc95f
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 62 deletions.
10 changes: 4 additions & 6 deletions lib/router/route-info.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface Route {
buildRouteInfoMetadata?(): unknown;
}

export type Continuation = () => PromiseLike<boolean> | boolean;
export type Continuation = () => boolean;

export interface RouteInfo {
readonly name: string;
Expand Down Expand Up @@ -376,11 +376,9 @@ export default class InternalRouteInfo<T extends Route> {
}

private checkForAbort<T>(shouldContinue: Continuation, value: T) {
return Promise.resolve(shouldContinue()).then(function () {
// We don't care about shouldContinue's resolve value;
// pass along the original value passed to this fn.
return value;
}, null);
shouldContinue();

return value;
}

private stashResolvedModel(transition: InternalTransition<T>, resolvedModel: Dict<unknown>) {
Expand Down
47 changes: 23 additions & 24 deletions lib/router/transition-state.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Promise } from 'rsvp';
import { Dict } from './core';
import InternalRouteInfo, { Continuation, Route } from './route-info';
import InternalRouteInfo, { Continuation, Route, ResolvedRouteInfo } from './route-info';
import Transition from './transition';
import { forEach, promiseLabel } from './utils';

Expand Down Expand Up @@ -42,40 +42,41 @@ export default class TransitionState<T extends Route> {
// The prelude RSVP.resolve() asyncs us into the promise land.
return Promise.resolve(null, this.promiseLabel('Start transition'))
.then(resolveOneRouteInfo, null, this.promiseLabel('Resolve route'))
.catch(handleError, this.promiseLabel('Handle error'));
.catch(handleError, this.promiseLabel('Handle error'))
.then(() => {
return currentState;
});

function innerShouldContinue() {
return Promise.resolve(
shouldContinue(),
currentState.promiseLabel('Check if should continue')
).catch(function (reason) {
if (shouldContinue()) {
return true;
} else {
// We distinguish between errors that occurred
// during resolution (e.g. before"Model/model/afterModel),
// and aborts due to a rejecting promise from shouldContinue().
wasAborted = true;
return Promise.reject(reason);
}, currentState.promiseLabel('Handle abort'));
throw new Error('Transition aborted');
}
}

function handleError(error: Error) {
function handleError(error: Error): never {
// This is the only possible
// reject value of TransitionState#resolve
let routeInfos = currentState.routeInfos;
let errorHandlerIndex =
transition.resolveIndex >= routeInfos.length
? routeInfos.length - 1
: transition.resolveIndex;
return Promise.reject(
new TransitionError(
error,
currentState.routeInfos[errorHandlerIndex].route!,
wasAborted,
currentState
)

throw new TransitionError(
error,
currentState.routeInfos[errorHandlerIndex].route!,
wasAborted,
currentState
);
}

function proceed(resolvedRouteInfo: InternalRouteInfo<T>): Promise<InternalRouteInfo<T>> {
function proceed(resolvedRouteInfo: ResolvedRouteInfo<T>): void | Promise<void> {
let wasAlreadyResolved = currentState.routeInfos[transition.resolveIndex].isResolved;

// Swap the previously unresolved routeInfo with
Expand All @@ -97,18 +98,16 @@ export default class TransitionState<T extends Route> {

// Proceed after ensuring that the redirect hook
// didn't abort this transition by transitioning elsewhere.
return innerShouldContinue().then(
resolveOneRouteInfo,
null,
currentState.promiseLabel('Resolve route')
);
if (innerShouldContinue()) {
return resolveOneRouteInfo();
}
}

function resolveOneRouteInfo(): TransitionState<T> | Promise<any> {
function resolveOneRouteInfo(): void | Promise<void> {
if (transition.resolveIndex === currentState.routeInfos.length) {
// This is is the only possible
// fulfill value of TransitionState#resolve
return currentState;
return;
}

let routeInfo = currentState.routeInfos[transition.resolveIndex];
Expand Down
8 changes: 1 addition & 7 deletions lib/router/transition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,7 @@ export default class Transition<T extends Route> implements Partial<Promise<T>>

this.sequence = router.currentSequence++;
this.promise = state
.resolve(() => {
if (this.isAborted) {
return Promise.reject(false, promiseLabel('Transition aborted - reject'));
}

return Promise.resolve(true);
}, this)
.resolve(() => !this.isAborted, this)
.catch((result: TransitionError) => {
return Promise.reject(this.router.transitionDidError(result, this));
}, promiseLabel('Handle Abort'));
Expand Down
29 changes: 14 additions & 15 deletions tests/route_info_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,9 @@ import RouteInfo, {
} from 'router/route-info';
import InternalTransition from 'router/transition';
import URLTransitionIntent from 'router/transition-intent/url-transition-intent';
import { reject, resolve } from 'rsvp';
import { resolve } from 'rsvp';
import { createHandler, createHandlerInfo, module, test, TestRouter } from './test_helpers';

function noop() {
return resolve(true);
}

module('RouteInfo');

test('ResolvedRouteInfo resolve to themselves', function (assert) {
Expand Down Expand Up @@ -43,9 +39,10 @@ test('RouteInfo can be aborted mid-resolve', function (assert) {

let routeInfo = createHandlerInfo('stub');

function abortResolve() {
function abortResolve(): boolean {
assert.ok(true, 'abort was called');
return reject('LOL');

throw new Error('foo');
}

routeInfo.resolve(abortResolve, {} as Transition).catch(function (error: Error) {
Expand Down Expand Up @@ -81,7 +78,7 @@ test('RouteInfo#resolve runs beforeModel hook on handler', function (assert) {
}),
});

routeInfo.resolve(noop, transition as Transition);
routeInfo.resolve(() => true, transition as Transition);
});

test('RouteInfo#resolve runs getModel hook', function (assert) {
Expand All @@ -95,7 +92,7 @@ test('RouteInfo#resolve runs getModel hook', function (assert) {
},
});

routeInfo.resolve(noop, transition as Transition);
routeInfo.resolve(() => true, transition as Transition);
});

test('RouteInfo#resolve runs afterModel hook on handler', function (assert) {
Expand All @@ -118,7 +115,7 @@ test('RouteInfo#resolve runs afterModel hook on handler', function (assert) {
});

routeInfo
.resolve(noop, transition as Transition)
.resolve(() => true, transition as Transition)
.then(function (resolvedRouteInfo: RouteInfo<Route>) {
assert.equal(resolvedRouteInfo.context, model, 'RouteInfo resolved with correct model');
});
Expand Down Expand Up @@ -146,7 +143,7 @@ test('UnresolvedRouteInfoByParam gets its model hook called', function (assert)
})
);

routeInfo.resolve(noop, transition as Transition);
routeInfo.resolve(() => true, transition as Transition);
});

test('UnresolvedRouteInfoByObject does NOT get its model hook called', function (assert) {
Expand All @@ -166,10 +163,12 @@ test('UnresolvedRouteInfoByObject does NOT get its model hook called', function
resolve({ name: 'dorkletons' })
);

routeInfo.resolve(noop, {} as Transition).then(function (resolvedRouteInfo: RouteInfo<Route>) {
// @ts-ignore
assert.equal(resolvedRouteInfo.context!.name, 'dorkletons');
});
routeInfo
.resolve(() => true, {} as Transition)
.then(function (resolvedRouteInfo: RouteInfo<Route>) {
// @ts-ignore
assert.equal(resolvedRouteInfo.context!.name, 'dorkletons');
});
});

test('RouteInfo.find', function (assert) {
Expand Down
15 changes: 5 additions & 10 deletions tests/transition_state_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
UnresolvedRouteInfoByParam,
} from 'router/route-info';
import TransitionState, { TransitionError } from 'router/transition-state';
import { Promise, reject, resolve } from 'rsvp';
import { Promise, resolve } from 'rsvp';
import {
createHandler,
createHandlerInfo,
Expand Down Expand Up @@ -54,7 +54,7 @@ test("#resolve delegates to handleInfo objects' resolve()", function (assert) {

function keepGoing() {
assert.ok(true, 'continuation function was called');
return Promise.resolve(false);
return true;
}

state.resolve(keepGoing, {} as Transition).then(function (result: TransitionState<Route>) {
Expand All @@ -63,7 +63,7 @@ test("#resolve delegates to handleInfo objects' resolve()", function (assert) {
});

test('State resolution can be halted', function (assert) {
assert.expect(2);
assert.expect(1);

let state = new TransitionState();

Expand All @@ -81,11 +81,10 @@ test('State resolution can be halted', function (assert) {
];

function keepGoing() {
return reject('NOPE');
return false;
}

state.resolve(keepGoing, {} as Transition).catch(function (reason: TransitionError) {
assert.equal(reason.error, 'NOPE');
assert.ok(reason.wasAborted, 'state resolution was correctly marked as aborted');
});

Expand Down Expand Up @@ -118,12 +117,8 @@ test('Integration w/ HandlerInfos', function (assert) {
new UnresolvedRouteInfoByObject(router, 'bar', ['bar_id'], resolve(barModel)),
];

function noop() {
return Promise.resolve(false);
}

state
.resolve(noop, transition as Transition)
.resolve(() => true, transition as Transition)
.then(function (result: TransitionState<Route>) {
let models = [];
for (let i = 0; i < result.routeInfos.length; i++) {
Expand Down

0 comments on commit e8bc95f

Please sign in to comment.