-
Notifications
You must be signed in to change notification settings - Fork 594
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
Conversation
src/selectors.js
Outdated
const getAction = state => toJS(getIn(state, ['router', 'action'])) | ||
const { get, toJS } = structure | ||
const getRouter = state => { | ||
const router = toJS(get(state, 'router')) |
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.
Can we just use getIn(state, 'router')
here? So, we don't need to create addition get
.
src/selectors.js
Outdated
@@ -1,9 +1,15 @@ | |||
import { matchPath } from "react-router" | |||
import isObject from 'lodash/isObject' |
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.
isObject
is quite simple, so, we can define isObject
here and don't need to maintain external dependency.
const isObject = (value) => (value != null && (typeof value === 'object' || typeof value === 'function'))
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.
@mattvague Can you address the comments? Thanks!
@supasate Hey! Sorry been super busy. Doing this right now |
src/selectors.js
Outdated
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer keeping it here for backward compatibility. By the way, _getLocation
under getLocation
seems redundant to me. getIn(getRouter(state), ['location'])
seems to be more straightforward.
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.
Yeah that's fine, was just trying to eliminate duplication but I'll change it
- Use getIn instead of get - Use getRouter and getLocation in ConnectedRouter - Remove dependency on lodash/isObject
9ed9c38
to
0c2e818
Compare
src/selectors.js
Outdated
const getAction = state => toJS(getIn(state, ['router', 'action'])) | ||
|
||
const _getLocation = state => getIn(state, ['location']) | ||
const _getAction = state => getIn(state, ['action']) |
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.
src/selectors.js
Outdated
|
||
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I think it's good to check :)
Get rid of inner fucnctions
@supasate Addressed your comments |
test/selectors.test.js
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo. Could
. Hmm..why did the test pass?
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Thanks for working on this! |
#174