feat($onBeforeChange): short-circuit redirecting + refactor middlewar…

…eCreateNotFoundAction + tests

__test-helpers__/setup.js

@@ -7,7 +7,8 @@ export default (
 ) => {
   const routesMap = {
     FIRST: '/first',
-    SECOND: '/second/:param'
+    SECOND: '/second/:param',
+    THIRD: '/third'
   }
 
   const history = createHistory({

__tests__/__snapshots__/connectRoutes.js.snap

@@ -128,13 +128,56 @@ Object {
     "routesMap": Object {
       "FIRST": "/first",
       "SECOND": "/second/:param",
+      "THIRD": "/third",
     },
     "type": "SECOND",
   },
   "title": "title: SECOND",
 }
 `;
 
+exports[`middleware if onBeforeChange dispatches redirect, route changes with kind === "redirect" 1`] = `
+Object {
+  "hasSSR": undefined,
+  "history": undefined,
+  "kind": "redirect",
+  "pathname": "/third",
+  "payload": Object {},
+  "prev": Object {
+    "pathname": "/first",
+    "payload": Object {},
+    "type": "FIRST",
+  },
+  "routesMap": Object {
+    "FIRST": "/first",
+    "SECOND": "/second/:param",
+    "THIRD": "/third",
+  },
+  "type": "THIRD",
+}
+`;
+
+exports[`middleware onBeforeChange redirect on server results in 1 history entry 1`] = `
+Object {
+  "hasSSR": true,
+  "history": undefined,
+  "kind": "redirect",
+  "pathname": "/third",
+  "payload": Object {},
+  "prev": Object {
+    "pathname": "/first",
+    "payload": Object {},
+    "type": "FIRST",
+  },
+  "routesMap": Object {
+    "FIRST": "/first",
+    "SECOND": "/second/:param",
+    "THIRD": "/third",
+  },
+  "type": "THIRD",
+}
+`;
+
 exports[`middleware user dispatches NOT_FOUND and middleware adds missing info to action 1`] = `
 Object {
   "meta": Object {

__tests__/connectRoutes.js

@@ -1,4 +1,4 @@
-import { createStore, applyMiddleware } from 'redux'
+import { createStore, applyMiddleware, compose } from 'redux'
 import { createMemoryHistory } from 'history'
 
 import setup from '../__test-helpers__/setup'
@@ -174,6 +174,61 @@ describe('middleware', () => {
     expect(onBeforeChange).toHaveBeenCalled()
   })
 
+  it('if onBeforeChange dispatches redirect, route changes with kind === "redirect"', () => {
+    const onBeforeChange = jest.fn((dispatch, getState, action) => {
+      if (action.type !== 'SECOND') return
+      const act = redirect({ type: 'THIRD' })
+      dispatch(act)
+    })
+    const { middleware, reducer, enhancer, history } = setup('/first', {
+      onBeforeChange
+    })
+    const middlewares = applyMiddleware(middleware)
+    const enhancers = compose(enhancer, middlewares)
+    const rootReducer = (state = {}, action = {}) => ({
+      location: reducer(state.location, action),
+      title: action.type
+    })
+
+    const store = createStore(rootReducer, enhancers)
+    store.dispatch({ type: 'SECOND', payload: { param: 'bar' } })
+
+    const { location } = store.getState()
+    expect(location.kind).toEqual('redirect') /*? store.getState() */
+    expect(location.type).toEqual('THIRD')
+    expect(history.entries.length).toEqual(2)
+    expect(location).toMatchSnapshot()
+    expect(onBeforeChange).toHaveBeenCalled()
+  })
+
+  it('onBeforeChange redirect on server results in 1 history entry', () => {
+    window.SSRtest = true
+
+    const onBeforeChange = jest.fn((dispatch, getState, action) => {
+      if (action.type !== 'SECOND') return
+      const act = redirect({ type: 'THIRD' })
+      dispatch(act)
+    })
+    const { middleware, reducer, enhancer, history } = setup('/first', {
+      onBeforeChange
+    })
+    const middlewares = applyMiddleware(middleware)
+    const enhancers = compose(enhancer, middlewares)
+    const rootReducer = (state = {}, action = {}) => ({
+      location: reducer(state.location, action),
+      title: action.type
+    })
+
+    const store = createStore(rootReducer, enhancers)
+    store.dispatch({ type: 'SECOND', payload: { param: 'bar' } })
+
+    const { location } = store.getState()
+    expect(history.entries.length).toEqual(1) // what we are testing for
+    expect(location).toMatchSnapshot()
+
+    window.SSRtest = false
+  })
+
   it('calls onAfterChange handler on route change', () => {
     const onAfterChange = jest.fn()
     const { middleware, reducer } = setup('/first', { onAfterChange })

package.json

@@ -2,7 +2,7 @@
   "name": "redux-first-router",
   "version": "0.0.0-development",
   "description": "think of your app in states not routes (and, yes, while keeping the address bar in sync)",
-  "main": "dist/index.js",
+  "main": "src/index.js",
   "scripts": {
     "build": "babel src -d dist",
     "build:umd": "BABEL_ENV=commonjs NODE_ENV=development webpack src/index.js dist/redux-first-router.js",

src/action-creators/middlewareCreateNotFoundAction.js

@@ -0,0 +1,34 @@
+// @flow
+import type { Location, Action, LocationState, History } from '../flow-types'
+import nestAction from '../pure-utils/nestAction'
+import { NOT_FOUND } from '../index'
+
+export default (
+  action: Object,
+  location: LocationState,
+  prevLocation: Location,
+  history: History,
+  notFoundPath: string
+): Action => {
+  const { payload } = action
+
+  const meta = action.meta
+  const prevPath = location.pathname
+  const kind =
+    (meta && meta.location && meta.location.kind) || // use case: kind === 'redirect'
+    (location.kind === 'load' && 'load') ||
+    'push'
+  const pathname =
+    (meta && meta.notFoundPath) ||
+    (kind === 'redirect' && notFoundPath) ||
+    prevPath ||
+    '/'
+
+  return nestAction(
+    pathname,
+    { type: NOT_FOUND, payload },
+    prevLocation,
+    history,
+    kind
+  )
+}

src/action-creators/redirect.js

@@ -2,4 +2,16 @@
 import type { Action } from '../flow-types'
 import setKind from '../pure-utils/setKind'
 
-export default (action: Action) => setKind(action, 'redirect')
+export default (action: Action, type?: string, payload?: any) => {
+  action = setKind(action, 'redirect')
+
+  if (type) {
+    action.type = type
+  }
+
+  if (payload) {
+    action.payload = payload
+  }
+
+  return action
+}

src/connectRoutes.js

@@ -12,6 +12,8 @@ import createThunk from './pure-utils/createThunk'
 
 import historyCreateAction from './action-creators/historyCreateAction'
 import middlewareCreateAction from './action-creators/middlewareCreateAction'
+import middlewareCreateNotFoundAction
+  from './action-creators/middlewareCreateNotFoundAction'
 
 import createLocationReducer, {
   getInitialState
@@ -201,27 +203,12 @@ export default (
     }
     else if (action.type === NOT_FOUND && !isLocationAction(action)) {
       // user decided to dispatch `NOT_FOUND`, so we fill in the missing location info
-      const { location } = store.getState()
-      const { payload } = action
-
-      const meta = action.meta
-      const kind =
-        (meta && meta.location && meta.location.kind) || // likely kind === 'redirect'
-        (location.kind === 'load' && 'load') ||
-        'push'
-      const prevPath = location.pathname
-      const pathname =
-        (meta && meta.notFoundPath) ||
-        (kind === 'redirect' && notFoundPath) ||
-        prevPath ||
-        '/'
-
-      action = nestAction(
-        pathname,
-        { type: NOT_FOUND, payload },
+      action = middlewareCreateNotFoundAction(
+        action,
+        store.getState().location,
         prevLocation,
         history,
-        kind
+        notFoundPath
       )
     }
     else if (route && !isLocationAction(action)) {
@@ -241,12 +228,13 @@ export default (
     }
 
     // DISPATCH LIFECYLE:
-
+    let skip
     if ((route || action.type === NOT_FOUND) && action.meta) {
       // satisify flow with `action.meta` check
-      _beforeRouteChange(store, next, history, action)
+      skip = _beforeRouteChange(store, next, history, action)
     }
 
+    if (skip) return
     const nextAction = next(action) // DISPATCH
 
     if (route || action.type === NOT_FOUND) {
@@ -265,8 +253,36 @@ export default (
     const location = action.meta.location
 
     if (onBeforeChange) {
-      const dispatch = middleware(store)(next) // re-create middleware's position in chain
+      let skip
+
+      const dispatch = (action: Object) => {
+        if (
+          action &&
+          action.meta &&
+          action.meta.location &&
+          action.meta.location.kind === 'redirect'
+        ) {
+          skip = true
+          const isHistoryChange = location.current.pathname === currentPathname
+
+          // In this unique scenario, the action won't in fact be treated as a
+          // redirect since the initial action is never dispatched. If it is
+          // an action resulting from pressing the browser buttons, it will
+          // do a replace just like a redirect is meant to, since the location
+          // change is unavoidable and happens before the middleware. On the
+          // server, a redirect is always dispatched since its needed to detect
+          // whether to call `res.redirect`. In that case history is irrelevant.
+          if (!isHistoryChange && !isServer()) {
+            history.push(action.meta.location.pathname) // this will be replaced since it's a redirect
+          }
+        }
+
+        const dispatch = middleware(store)(next) // re-create middleware's position in chain
+        dispatch(action)
+      }
+
       onBeforeChange(dispatch, store.getState, action)
+      if (skip) return true
     }
 
     prevState = store.getState()[locationKey]