[FEAT]: Add selectors #98
Conversation
josepot
force-pushed
the
feat/selectors
branch
2 times, most recently
from
abd4990
to
463ed5c
Compare
src/selectors.js
Outdated
|
|
||
| const createSelectors = (structure) => { | ||
| const { getIn, toJS } = structure | ||
|
|
There was a problem hiding this comment.
nit: No new line needed for variable declaration.
src/selectors.js
Outdated
| const getAction = state => toJS(getIn(state, ['router', 'action'])) | ||
| const createMatchSelector = path => { | ||
| let lastPathname = null | ||
| let lastMatch = null |
| const match = matchPath(pathname, path) | ||
| if (!match || !lastMatch || match.url !== lastMatch.url) { | ||
| lastMatch = match | ||
| } |
src/selectors.js
Outdated
|
|
||
| const getLocation = state => toJS(getIn(state, ['router', 'location'])) | ||
| const getAction = state => toJS(getIn(state, ['router', 'action'])) | ||
| const createMatchSelector = path => { |
There was a problem hiding this comment.
Can you add some comments about the behavior of the matchSelector? So, it will be easier to understand for anyone who read the code. Ex. I need to read twice before I realized that lastPathname and lastMatch are inside a closure and their values are kept in memory, not re-initialized to null everytime.
test/selectors.test.js
Outdated
| }) | ||
| }) | ||
|
|
||
| describe("getLocation", () => { |
test/selectors.test.js
Outdated
| }) | ||
|
|
||
| describe("getLocation", () => { | ||
| it("gets the location from the state", () => { |
| expect(() => matchSelector(state)).not.toThrow() | ||
| }) | ||
|
|
||
| it("does not update if the match is the same", () => { |
There was a problem hiding this comment.
For all test cases, instead of comparing match1 with match2, I think it will be easier to understand if we compare each one against its expected value.
|
Hi @supasate ! Thanks for the review, I rushed this a bit when I created the PR, sorry about that. I have addressed all your comments, please let me know if this look "mergeable" now or if you want me to make more changes. |
|
LGTM. Thanks for your contribution! |
Hi and thanks for this library!
I'm migrating from react-router-redux and the only thing that I'm really missing is its selectors... Specially the
createMatchSelector. So, I have taken the ones fromreact-router-reduxand I have migrated them (with their corresponding tests) so that they can work in this library.I would really appreciate it if you could add them to
connected-react-router.