Skip to content

Commit

Permalink
Fix review issues:
Browse files Browse the repository at this point in the history
- Use getIn instead of get
- Use getRouter and getLocation in ConnectedRouter
- Remove dependency on lodash/isObject
  • Loading branch information
mattvague committed Nov 12, 2018
1 parent 7d1421a commit 0c2e818
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 33 deletions.
10 changes: 6 additions & 4 deletions src/ConnectedRouter.js
Expand Up @@ -3,9 +3,11 @@ import PropTypes from 'prop-types'
import { connect } from 'react-redux'
import { Router } from 'react-router'
import { onLocationChanged } from './actions'
import createSelectors from './selectors'

const createConnectedRouter = (structure) => {
const { getIn, toJS } = structure
const { getIn } = structure
const { getRouter, getLocation } = createSelectors(structure)
/*
* ConnectedRouter listens to a history object passed from props.
* When history is changed, it dispatches action to redux store.
Expand All @@ -26,7 +28,7 @@ const createConnectedRouter = (structure) => {
pathname: pathnameInStore,
search: searchInStore,
hash: hashInStore,
} = toJS(getIn(context.store.getState(), ['router', 'location']))
} = getLocation(context.store.getState())
// Extract history's location
const {
pathname: pathnameInHistory,
Expand Down Expand Up @@ -102,8 +104,8 @@ const createConnectedRouter = (structure) => {
}

const mapStateToProps = state => ({
action: getIn(state, ['router', 'action']),
location: getIn(state, ['router', 'location']),
action: getIn(getRouter(state), ['action']),
location: getIn(getRouter(state), ['location']),
})

const mapDispatchToProps = dispatch => ({
Expand Down
18 changes: 11 additions & 7 deletions src/selectors.js
@@ -1,15 +1,19 @@
import { matchPath } from "react-router"
import isObject from 'lodash/isObject'

const createSelectors = (structure) => {
const { get, toJS } = structure
const { getIn, toJS } = structure

const _getLocation = state => getIn(state, ['location'])
const _getAction = state => getIn(state, ['action'])
const isRouter = (value) => value != null && typeof value === 'object' && _getLocation(value) && _getAction(value)

const getRouter = state => {
const router = toJS(get(state, 'router'))
if (!isObject(router)) { throw 'Could not find router reducer in state tree. Are you sure it is mounted under "router"?' }
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(get(getRouter(state), 'location'))
const getAction = state => toJS(get(getRouter(state), 'action'))
const getLocation = state => toJS(_getLocation(getRouter(state)))
const getAction = state => toJS(_getAction(getRouter(state)))

// It only makes sense to recalculate the `matchPath` whenever the pathname
// of the location changes. That's why `createMatchSelector` memoizes
Expand All @@ -33,7 +37,7 @@ const createSelectors = (structure) => {
}
}

return {getLocation, getAction, createMatchSelector}
return {getLocation, getAction, getRouter, createMatchSelector}
}

export default createSelectors
9 changes: 0 additions & 9 deletions src/structure/immutable/get.js

This file was deleted.

2 changes: 0 additions & 2 deletions src/structure/immutable/index.js
@@ -1,12 +1,10 @@
import { Iterable, fromJS } from 'immutable'
import getIn from './getIn'
import get from './get'

const structure = {
fromJS: jsValue => fromJS(jsValue, (key, value) =>
Iterable.isIndexed(value) ? value.toList() : value.toMap()),
getIn,
get,
merge: (state, payload) => state.merge(payload),
toJS: value => Iterable.isIterable(value) ? value.toJS() : value,
}
Expand Down
5 changes: 0 additions & 5 deletions src/structure/plain/get.js

This file was deleted.

2 changes: 0 additions & 2 deletions src/structure/plain/index.js
@@ -1,10 +1,8 @@
import getIn from './getIn'
import get from './get'

const structure = {
fromJS: value => value,
getIn,
get,
merge: (state, payload) => ({ ...state, ...payload }),
toJS: value => value,
}
Expand Down
2 changes: 0 additions & 2 deletions src/structure/seamless-immutable/index.js
@@ -1,13 +1,11 @@
import SeamlessImmutable from 'seamless-immutable'
import getIn from '../plain/getIn'
import get from '../plain/get'

const { static: Immutable } = SeamlessImmutable

const structure = {
fromJS: value => Immutable.from(value),
getIn,
get,
merge: (state, payload) => Immutable.merge(state, payload),
toJS: value => Immutable.asMutable(value)
}
Expand Down
19 changes: 17 additions & 2 deletions test/selectors.test.js
Expand Up @@ -23,7 +23,7 @@ describe("selectors", () => {
store = createStore(reducer)
})

describe("when router not found", () => {
describe("when router not found under 'router' key", () => {
beforeEach(() => {
const reducer = combineReducers({
notTheRouter: connectRouter(history)
Expand All @@ -34,7 +34,22 @@ describe("selectors", () => {
it("throws helpful error", () => {
store.dispatch(push('/'))
const state = store.getState()
expect(() => getLocation(state)).toThrowError('Could not find router reducer in state tree. Are you sure it is mounted under "router"?')
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"')
})
})

Expand Down

0 comments on commit 0c2e818

Please sign in to comment.