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

Split history syncing from action creators #259

Merged
merged 38 commits into from
Feb 17, 2016
Merged

Split history syncing from action creators #259

merged 38 commits into from
Feb 17, 2016

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Feb 5, 2016

Big thanks to @gaearon for opening up this can of worms and writing a bunch of code to fix it!

Here's the history (pun intended) of what led to this:
#252 (comment) -> #255 -> #257 -> reduxjs/redux#1362 (PR)

A big giant chunk of this code is copypasta from Dan's work on that Redux PR. I just mashed some buttons until it looked right.

Changes

The provided action creators and middleware are now separate from the history<->state syncing function. For the vast majority of cases, using action creators to trigger navigation is obsoleted by React Router's new history singletons provided in 2.0. Building this functionality in by default and coupling it to our history syncing logic no longer makes sense.

We now sync by enhancing the history instance to listen for navigation events and dispatch those into the store. The enhanced history has its listen method overridden to respond to store changes, rather than directly to navigation events. When this history is provided to <Router>, the router will listen to it and receive these store changes. This means if we time travel with the store, the router will receive those store changes and update based on the location in the store, instead of what the browser says. Normal navigation events (hitting your browser back/forward buttons, telling a history singleton to push a location) flow through the history's listener like normal, so all the usual stuff works A-OK.

TODOs

  • Add tests
  • Update the documentation
  • Find bugs/edge cases and fix them as best we can.
  • Fix server side rendering

Fixes #193
Fixes #252

Getting the easy parts done first.
See reduxjs/redux#1362

Some small modifications and commenting added. Absolutely needs tests!
Not that bad. In fact, it's mostly stuff removed! :)
Fix Firefox with babel-polyfill.
// Update address bar to reflect store state
isTimeTraveling = true
currentLocation = locationInStore
history.transitionTo(Object.assign({},
Copy link
Member

Choose a reason for hiding this comment

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

Let's use spread here so we don't have to worry about polyfilling Object.assign(). Babel still doesn't replace this call automatically does it?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does. And Travis' version of Firefox (31) is old enough not to have Object.assign built in. So it doesn't really matter either way, but I will probably switch it to a spread. Haven't made it this far down in the file yet 😄

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need this

@gaearon
Copy link
Member

gaearon commented Feb 5, 2016

Thank you very much for getting this moving! I might be able to find some time to contribute in the next week but this is already shaping up nicely 🙃

@ryanflorence
Copy link

👏

@webmasterkai
Copy link
Contributor

Can someone confirm for me that this is just syncing state with rackt/history and the use of react-router is secondary? Any objections to me making a pull request against the synchronicity with the change suggestions above?

@gaearon
Copy link
Member

gaearon commented Feb 6, 2016

Can someone confirm for me that this is just syncing state with rackt/history and the use of react-router is secondary?

This is correct. react-router in the library name is just for marketing purposes because that’s what people google for.

Any objections to me making a pull request against the synchronicity with the change suggestions above?

I’m pretty sure any contributions are welcome!

@webmasterkai
Copy link
Contributor

Was it decided somewhere how defaultState based on current location should be set? This is something that was handled before that seems to be missing now. Having to manually do the following seems a bit inelegant.

const history = createHistory()
function initLocation() {
  let locationBeforeTransitions
  history.listen(loc => locationBeforeTransitions = loc)()
  return locationBeforeTransitions
}
const initialState = {
  routing: {
    locationBeforeTransitions: initLocation()
  }
}
const store = createStore(reducer, initialState)

Is it the job for a store enhancer?

Yes, it is set with an action on invocation of syncHistoryWithStore(). However, dispatching an action afterward feels like a hack when it's possible to set initialState correctly.

@webmasterkai webmasterkai mentioned this pull request Feb 6, 2016
@romseguy
Copy link

romseguy commented Feb 9, 2016

Sorry for jumping in without reading the whole thread but I gave 4.0.0-beta.1 a spin along with RR2.0.0. I'm using SSR and accessing e.g /login is correctly matched and rendered on the server, but I am redirected to / shortly after. Accessing the route from view-source:http://localhost:3000/login here's the store's routing state I get from the server:

{
  "routing": {
    "locationBeforeTransitions": {
      "pathname": "\/",
      "search": "",
      "hash": "",
      "state": null,
      "action": "POP",
      "key": "0t38e1",
      "query": {

      },
      "$searchBase": {
        "search": "",
        "searchBase": ""
      }
    }
  }
}

I'm not familiar with the internals but I'd guess the store isn't synced with history state after /login is matched.

My (trimmed down) implementation:

// server.js
import { createMemoryHistory } from 'react-router'

const { store, syncedHistory } = createStoreWithHistory(createMemoryHistory())
const location = syncedHistory.createLocation(req.url)
match({routes, location}, (error, redirectLocation, routingProps) => {/* ... */})
// client.js
import { browserHistory } from 'react-router'

const { store, syncedHistory } = createStoreWithHistory(browserHistory, window.__data)

ReactDOM.render((
    <Provider store={store}>
      <Router history={syncedHistory}>
        {routes}
      </Router>
    </Provider>
  ),
  node
)
// createStoreWithHistory.js
import { syncHistoryWithStore } from 'react-router-redux'
import { createStore } from 'redux'

function createStoreWithHistory(history, initialState) {
  const store = createStore(reducer, initialState)
  return {store, syncedHistory: syncHistoryWithStore(history, store)}
}

@timdorr
Copy link
Member Author

timdorr commented Feb 10, 2016

@SimenB I can look at IE8 compatibility and implement Redux's solution.

Yes, you can keep using the plain history singleton from React Router. The only difference between the two is the listen implementation, so push and the rest of it is the exact same function.

@romseguy I haven't done any SSR testing yet. But try this instead:

import { createMemoryHistory } from 'react-router'

const { store, syncedHistory } = createStoreWithHistory(createMemoryHistory())
match({routes, location: req.url, history: syncedHistory}, (error, redirectLocation, routingProps) => {/* ... */})

You definitely need to pass the enhanced history into match, otherwise it doesn't get used in the router.

@romseguy
Copy link

Same problem.

@svrcekmichal
Copy link
Contributor

Having the same issue as @romseguy , created this issue #284

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

@romseguy & @svrcekmichal This was fixed by properly seeding our memory history with the right path:

const memoryHistory = createMemoryHistory(req.path)

I'll have to check that our SSR docs on react-router are consistent.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2016

I haven't seen any serious issues come up, so I'm going to merge this into master. I've cut a 3.0.x branch and will leave that as default for a while until 4.0.0 goes final.

While there haven't been any changes to the code, I'm going to push an rc1 release shortly. Hopefully, that will signal to more people to give it a try. It works well! That should root out some further bugs and we can make this thing about as good as it can be.

timdorr added a commit that referenced this pull request Feb 17, 2016
Split history syncing from action creators
@timdorr timdorr merged commit 8d5b39b into master Feb 17, 2016
@timdorr timdorr deleted the synchronicity branch February 17, 2016 05:37
@romseguy
Copy link

Thank you @timdorr and everyone else, works great now.

@transtone
Copy link

how to use it, is there a doc?

@reactjs reactjs deleted a comment from SherylHohman Feb 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restore router state First mutation