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

[added] onChange route hook #3108

Merged
merged 1 commit into from
Mar 21, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
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
71 changes: 51 additions & 20 deletions modules/TransitionUtils.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { loopAsync } from './AsyncUtils'
import warning from './routerWarning'

function createEnterHook(hook, route) {
return function (a, b, callback) {
hook.apply(route, arguments)
function createTransitionHook(hook, route, asyncArity) {
return function (...args) {
hook.apply(route, args)

if (hook.length < 3) {
if (hook.length < asyncArity) {
let callback = args[args.length - 1]
Copy link
Contributor

Choose a reason for hiding this comment

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

asyncArity - 1? To avoid dumb crap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch.

// Assume hook executes synchronously and
// automatically call the callback.
callback()
Expand All @@ -16,26 +17,22 @@ function createEnterHook(hook, route) {
function getEnterHooks(routes) {
return routes.reduce(function (hooks, route) {
if (route.onEnter)
hooks.push(createEnterHook(route.onEnter, route))
hooks.push(createTransitionHook(route.onEnter, route, 3))

return hooks
}, [])
}

/**
* Runs all onEnter hooks in the given array of routes in order
* with onEnter(nextState, replace, callback) and calls
* callback(error, redirectInfo) when finished. The first hook
* to use replace short-circuits the loop.
*
* If a hook needs to run asynchronously, it may use the callback
* function. However, doing so will cause the transition to pause,
* which could lead to a non-responsive UI if the hook is slow.
*/
export function runEnterHooks(routes, nextState, callback) {
const hooks = getEnterHooks(routes)
function getChangeHooks(routes) {
return routes.reduce(function (hooks, route) {
if (route.onChange)
hooks.push(createTransitionHook(route.onChange, route, 4))
return hooks
}, [])
}

if (!hooks.length) {
function runTransitionHooks(length, iter, callback) {
if (!length) {
callback()
return
}
Expand All @@ -59,8 +56,8 @@ export function runEnterHooks(routes, nextState, callback) {
redirectInfo = location
}

loopAsync(hooks.length, function (index, next, done) {
hooks[index](nextState, replace, function (error) {
loopAsync(length, function (index, next, done) {
iter(index, replace, function (error) {
if (error || redirectInfo) {
done(error, redirectInfo) // No need to continue.
} else {
Expand All @@ -70,6 +67,40 @@ export function runEnterHooks(routes, nextState, callback) {
}, callback)
}

/**
* Runs all onEnter hooks in the given array of routes in order
* with onEnter(nextState, replace, callback) and calls
* callback(error, redirectInfo) when finished. The first hook
* to use replace short-circuits the loop.
*
* If a hook needs to run asynchronously, it may use the callback
* function. However, doing so will cause the transition to pause,
* which could lead to a non-responsive UI if the hook is slow.
*/
export function runEnterHooks(routes, nextState, callback) {
const hooks = getEnterHooks(routes)
return runTransitionHooks(hooks.length, (index, replace, next) => {
hooks[index](nextState, replace, next)
}, callback)
}

/**
* Runs all onChange hooks in the given array of routes in order
* with onChange(prevState, nextState, replace, callback) and calls
* callback(error, redirectInfo) when finished. The first hook
* to use replace short-circuits the loop.
*
* If a hook needs to run asynchronously, it may use the callback
* function. However, doing so will cause the transition to pause,
* which could lead to a non-responsive UI if the hook is slow.
*/
export function runChangeHooks(routes, state, nextState, callback) {
const hooks = getChangeHooks(routes)
return runTransitionHooks(hooks.length, (index, replace, next) => {
hooks[index](state, nextState, replace, next)
}, callback)
}

/**
* Runs all onLeave hooks in the given array of routes in order.
*/
Expand Down
30 changes: 30 additions & 0 deletions modules/__tests__/transitionHooks-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ describe('When a router enters a branch', function () {
expect(nextState.routes).toContain(NewsFeedRoute)
expect(replace).toBeA('function')
},
onChange(prevState, nextState, replace) {
expect(this).toBe(NewsFeedRoute)
expect(prevState).toNotEqual(nextState)
expect(prevState.routes).toContain(NewsFeedRoute)
expect(nextState.routes).toContain(NewsFeedRoute)
expect(replace).toBeA('function')
},
onLeave() {
expect(this).toBe(NewsFeedRoute)
}
Expand Down Expand Up @@ -97,6 +104,12 @@ describe('When a router enters a branch', function () {
expect(nextState.routes).toContain(MessageRoute)
expect(replace).toBeA('function')
},
onChange(prevState, nextState, replace) {
expect(this).toBe(MessageRoute)
expect(prevState.routes).toContain(MessageRoute)
expect(nextState.routes).toContain(MessageRoute)
expect(replace).toBeA('function')
},
onLeave() {
expect(this).toBe(MessageRoute)
}
Expand All @@ -110,6 +123,13 @@ describe('When a router enters a branch', function () {
expect(nextState.routes).toContain(DashboardRoute)
expect(replace).toBeA('function')
},
onChange(prevState, nextState, replace) {
expect(this).toBe(DashboardRoute)
expect(prevState).toNotEqual(nextState)
expect(prevState.routes).toContain(DashboardRoute)
expect(nextState.routes).toContain(DashboardRoute)
expect(replace).toBeA('function')
},
onLeave() {
expect(this).toBe(DashboardRoute)
},
Expand Down Expand Up @@ -212,8 +232,11 @@ describe('When a router enters a branch', function () {
describe('and then navigates to the same branch, but with different params', function () {
it('calls the onLeave and onEnter hooks of all routes whose params have changed', function (done) {
const dashboardRouteLeaveSpy = spyOn(DashboardRoute, 'onLeave').andCallThrough()
const dashboardRouteChangeSpy = spyOn(DashboardRoute, 'onChange').andCallThrough()
const dashboardRouteEnterSpy = spyOn(DashboardRoute, 'onEnter').andCallThrough()

const messageRouteLeaveSpy = spyOn(MessageRoute, 'onLeave').andCallThrough()
const messageRouteChangeSpy = spyOn(MessageRoute, 'onChange').andCallThrough()
const messageRouteEnterSpy = spyOn(MessageRoute, 'onEnter').andCallThrough()
const history = createHistory('/messages/123')

Expand All @@ -226,6 +249,9 @@ describe('When a router enters a branch', function () {
function () {
expect(messageRouteLeaveSpy).toHaveBeenCalled('MessageRoute.onLeave was not called')
expect(messageRouteEnterSpy).toHaveBeenCalled('MessageRoute.onEnter was not called')
expect(messageRouteChangeSpy.calls.length).toEqual(0, 'DashboardRoute.onChange was called')

expect(dashboardRouteChangeSpy).toHaveBeenCalled('DashboardRoute.onChange was not called')
expect(dashboardRouteLeaveSpy.calls.length).toEqual(0, 'DashboardRoute.onLeave was called')
}
]
Expand All @@ -243,12 +269,16 @@ describe('When a router enters a branch', function () {
describe('and then the query changes', function () {
it('calls the onEnter hooks of all routes in that branch', function (done) {
const newsFeedRouteEnterSpy = spyOn(NewsFeedRoute, 'onEnter').andCallThrough()
const newsFeedRouteChangeSpy = spyOn(NewsFeedRoute, 'onChange').andCallThrough()
const history = useQueries(createHistory)('/inbox')

render(<Router history={history} routes={routes}/>, node, function () {
history.push({ pathname: '/news', query: { q: 1 } })
expect(newsFeedRouteChangeSpy.calls.length).toEqual(0, 'NewsFeedRoute.onChange was called')
expect(newsFeedRouteEnterSpy.calls.length).toEqual(1)

history.push({ pathname: '/news', query: { q: 2 } })
expect(newsFeedRouteChangeSpy).toHaveBeenCalled('NewsFeedRoute.onChange was not called')
expect(newsFeedRouteEnterSpy.calls.length).toEqual(1)
done()
})
Expand Down
23 changes: 19 additions & 4 deletions modules/computeChangedRoutes.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,23 @@ function routeParamsChanged(route, prevState, nextState) {
}

/**
* Returns an object of { leaveRoutes, enterRoutes } determined by
* Returns an object of { leaveRoutes, changeRoutes, enterRoutes } determined by
* the change from prevState to nextState. We leave routes if either
* 1) they are not in the next state or 2) they are in the next state
* but their params have changed (i.e. /users/123 => /users/456).
*
* leaveRoutes are ordered starting at the leaf route of the tree
* we're leaving up to the common parent route. enterRoutes are ordered
* from the top of the tree we're entering down to the leaf route.
*
* changeRoutes are any routes that didn't leave or enter during
* the transition.
*/
function computeChangedRoutes(prevState, nextState) {
const prevRoutes = prevState && prevState.routes
const nextRoutes = nextState.routes

let leaveRoutes, enterRoutes
let leaveRoutes, changeRoutes, enterRoutes
if (prevRoutes) {
leaveRoutes = prevRoutes.filter(function (route) {
return nextRoutes.indexOf(route) === -1 || routeParamsChanged(route, prevState, nextState)
Expand All @@ -34,16 +37,28 @@ function computeChangedRoutes(prevState, nextState) {
// onLeave hooks start at the leaf route.
leaveRoutes.reverse()

enterRoutes = nextRoutes.filter(function (route) {
return prevRoutes.indexOf(route) === -1 || leaveRoutes.indexOf(route) !== -1
enterRoutes = []
changeRoutes = []

nextRoutes.forEach(function (route) {
const isNew = prevRoutes.indexOf(route) === -1
const paramsChanged = leaveRoutes.indexOf(route) !== -1

if (isNew || paramsChanged)
enterRoutes.push(route)
else
changeRoutes.push(route)
})

} else {
leaveRoutes = []
changeRoutes = []
enterRoutes = nextRoutes
}

return {
leaveRoutes,
changeRoutes,
enterRoutes
}
}
Expand Down
52 changes: 31 additions & 21 deletions modules/createTransitionManager.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import warning from './routerWarning'
import { REPLACE } from 'history/lib/Actions'
import computeChangedRoutes from './computeChangedRoutes'
import { runEnterHooks, runLeaveHooks } from './TransitionUtils'
import { runEnterHooks, runChangeHooks, runLeaveHooks } from './TransitionUtils'
import { default as _isActive } from './isActive'
import getComponents from './getComponents'
import matchRoutes from './matchRoutes'
Expand Down Expand Up @@ -67,33 +67,43 @@ export default function createTransitionManager(history, routes) {
}

function finishMatch(nextState, callback) {
const { leaveRoutes, enterRoutes } = computeChangedRoutes(state, nextState)
const { leaveRoutes, changeRoutes, enterRoutes } = computeChangedRoutes(state, nextState)

runLeaveHooks(leaveRoutes)

// Tear down confirmation hooks for left routes
leaveRoutes.forEach(removeListenBeforeHooksForRoute)

runEnterHooks(enterRoutes, nextState, function (error, redirectInfo) {
if (error) {
callback(error)
} else if (redirectInfo) {
callback(null, createLocationFromRedirectInfo(redirectInfo))
} else {
// TODO: Fetch components after state is updated.
getComponents(nextState, function (error, components) {
if (error) {
callback(error)
} else {
// TODO: Make match a pure function and have some other API
// for "match and update state".
callback(null, null, (
state = { ...nextState, components })
)
}
})
}
// change and enter hooks are run in series
runChangeHooks(changeRoutes, state, nextState, (error, redirectInfo) => {
if (error || redirectInfo)
return handleErrorOrRedirect(error, redirectInfo)

runEnterHooks(enterRoutes, nextState, finishEnterHooks)
})

function finishEnterHooks(error, redirectInfo) {
if (error || redirectInfo)
return handleErrorOrRedirect(error, redirectInfo)

// TODO: Fetch components after state is updated.
getComponents(nextState, function (error, components) {
if (error) {
callback(error)
} else {
// TODO: Make match a pure function and have some other API
// for "match and update state".
callback(null, null, (
state = { ...nextState, components })
)
}
})
}

function handleErrorOrRedirect(error, redirectInfo) {
if (error) callback(error)
else callback(null, createLocationFromRedirectInfo(redirectInfo))
}
}

let RouteGuid = 1
Expand Down