Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

chore(history): update to history ^0.14.0 internally #131

Merged
merged 3 commits into from
Dec 23, 2015
Merged

chore(history): update to history ^0.14.0 internally #131

merged 3 commits into from
Dec 23, 2015

Conversation

justingreenberg
Copy link
Contributor

original discussion #89 i tried to keep changes as minimal as possible since it sounds like there may be a rewrite happening soon

i can also submit a separate PR for devtools now that it's stable if you want

@@ -88,7 +88,7 @@ export function syncReduxAndRouter(history, store, selectRouterState = SELECT_ST
const unsubscribeHistory = history.listen(location => {
const route = {
path: createPath(location),
state: location.state
state: location.state === null ? undefined : location.state
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this because tests failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @kjbekkelund! yes, the tests were failing due to the way history assigns a null value to state, so this was the most lightweight way to keep state undefined internally

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, maybe we should change the tests instead? Nice to be consistent with history, plus the code is simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justingreenberg If it isn't too much work changing the tests, maybe you could take a look when you've got the opportunity? No hurry — remember to relax and enjoy xmas :)

@kimjoar
Copy link
Collaborator

kimjoar commented Dec 19, 2015

Hi @justingreenberg, thanks for this! Only had a small question, otherwise I think this is good to go.

It would be great if you created a DevTools PR too!

@jlongster
Copy link
Member

👍

@justingreenberg
Copy link
Contributor Author

hey guys! @kjbekkelund ok so i removed the location.state expression and updated tests to expect state to be null after interacting with history... it's kinda awkward but it makes sense :)

let me know if you get around to reviewing, if not enjoy the holidays! 🎅

gets library back up to 100% coverage
@kimjoar
Copy link
Collaborator

kimjoar commented Dec 23, 2015

@justingreenberg Nice, thanks! I'll try to get this tested and merged tomorrow. Great work 💯

kimjoar added a commit that referenced this pull request Dec 23, 2015
@kimjoar kimjoar merged commit 4775cb1 into reactjs:master Dec 23, 2015
@justingreenberg justingreenberg deleted the history-update branch January 6, 2016 06:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants