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
Throw helpful error when router reducer not mounted under "router" #175
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,18 @@ import { matchPath } from "react-router" | |
|
||
const createSelectors = (structure) => { | ||
const { getIn, toJS } = structure | ||
const getLocation = state => toJS(getIn(state, ['router', 'location'])) | ||
const getAction = state => toJS(getIn(state, ['router', 'action'])) | ||
|
||
const _getLocation = state => getIn(state, ['location']) | ||
const _getAction = state => getIn(state, ['action']) | ||
const isRouter = (value) => value != null && typeof value === 'object' && _getLocation(value) && _getAction(value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @supasate Do you think there's value in doing this check to make sure the right object shape is mounted? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I think it's good to check :) |
||
|
||
const getRouter = state => { | ||
mattvague marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const router = toJS(getIn(state, ['router'])) | ||
if (!isRouter(router)) { throw 'Could not find router reducer in state tree, it must be mounted under "router"' } | ||
return router | ||
} | ||
const getLocation = state => toJS(_getLocation(getRouter(state))) | ||
const getAction = state => toJS(_getAction(getRouter(state))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems weird to me to be exporting these selectors always wrapped in toJS, is this something we could move somewhere immutable/seamless-immutable specific? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer keeping it here for backward compatibility. By the way, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's fine, was just trying to eliminate duplication but I'll change it |
||
|
||
// It only makes sense to recalculate the `matchPath` whenever the pathname | ||
// of the location changes. That's why `createMatchSelector` memoizes | ||
|
@@ -27,7 +37,7 @@ const createSelectors = (structure) => { | |
} | ||
} | ||
|
||
return {getLocation, getAction, createMatchSelector} | ||
return {getLocation, getAction, getRouter, createMatchSelector} | ||
} | ||
|
||
export default createSelectors |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,36 @@ describe("selectors", () => { | |
store = createStore(reducer) | ||
}) | ||
|
||
describe("when router not found under 'router' key", () => { | ||
beforeEach(() => { | ||
const reducer = combineReducers({ | ||
notTheRouter: connectRouter(history) | ||
}) | ||
store = createStore(reducer) | ||
}) | ||
|
||
it("throws helpful error", () => { | ||
store.dispatch(push('/')) | ||
const state = store.getState() | ||
expect(() => getLocation(state)).toThrowError('Could not find router reducer in state tree, it must be mounted under "router"') | ||
}) | ||
}) | ||
|
||
describe("when something else found under 'router' key", () => { | ||
beforeEach(() => { | ||
const reducer = combineReducers({ | ||
router: () => ({ some: 'thing' }) | ||
}) | ||
store = createStore(reducer) | ||
}) | ||
|
||
it("throws helpful error", () => { | ||
store.dispatch(push('/')) | ||
const state = store.getState() | ||
expect(() => getLocation(state)).toThrowError('ould not find router reducer in state tree, it must be mounted under "router"') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: typo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Found that Jest uses RegExp to test. So, if we pass a string, it will check if the input contains this specified substring or not (https://github.com/facebook/jest/blob/master/packages/expect/src/toThrowMatchers.js#L54). So, let's use toThrowError(/^Could not find router reducer in state tree, it must be mounted under "router"$/) for both test cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
}) | ||
}) | ||
|
||
describe("getLocation", () => { | ||
it("gets the location from the state", () => { | ||
const location = { pathname: "/", hash: '', search: '' } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this because it doesn't do anything beside getting a key at only one level depth.