Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify TransitionState resolution system. #306

Merged
merged 6 commits into from
Nov 9, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Simplify TransitionState resolution system.
rwjblue authored and stefanpenner committed Nov 6, 2020
commit e83c782d88d45c41cdfc5f6d1b23d74e15ad3481
10 changes: 4 additions & 6 deletions lib/router/route-info.ts
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ export interface Route {
buildRouteInfoMetadata?(): unknown;
}

export type Continuation = () => PromiseLike<boolean> | boolean;
export type Continuation = () => boolean;
rwjblue marked this conversation as resolved.
Show resolved Hide resolved

export interface RouteInfo {
readonly name: string;
@@ -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();
rwjblue marked this conversation as resolved.
Show resolved Hide resolved

return value;
}

private stashResolvedModel(transition: InternalTransition<T>, resolvedModel: Dict<unknown>) {
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';

@@ -42,40 +42,41 @@ export default class TransitionState<T extends Route> {
// The prelude RSVP.resolve() async moves 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');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given e83c782#r518934850 is this else unreachable?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess this is ultimately removed, so it doesn't matter.

}
}

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
@@ -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];
8 changes: 1 addition & 7 deletions lib/router/transition.ts
Original file line number Diff line number Diff line change
@@ -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'));
29 changes: 14 additions & 15 deletions tests/route_info_test.ts
Original file line number Diff line number Diff line change
@@ -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) {
@@ -43,9 +39,10 @@ test('RouteInfo can be aborted mid-resolve', function (assert) {

let routeInfo = createHandlerInfo('stub');

function abortResolve() {
function abortResolve(): boolean {
rwjblue marked this conversation as resolved.
Show resolved Hide resolved
assert.ok(true, 'abort was called');
return reject('LOL');

throw new Error('foo');
}

routeInfo.resolve(abortResolve, {} as Transition).catch(function (error: Error) {
@@ -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) {
@@ -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) {
@@ -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');
});
@@ -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) {
@@ -173,10 +170,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) {
15 changes: 5 additions & 10 deletions tests/transition_state_test.ts
Original file line number Diff line number Diff line change
@@ -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,
@@ -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>) {
@@ -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();

@@ -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');
});

@@ -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++) {