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

Restore router state #252

Closed
awinogradov opened this issue Feb 1, 2016 · 24 comments · Fixed by #259
Closed

Restore router state #252

awinogradov opened this issue Feb 1, 2016 · 24 comments · Fixed by #259
Labels

Comments

@awinogradov
Copy link

I use react-router-redux for sync history. I want to save the state of an application and restore it in the future. All components wich connected to redux work fine with restored state wich I transfer by initial state. But! Router doesn't understand :( What is wrong?

screen shot 2016-02-02 at 00 13 36

I got / ;(

@timdorr
Copy link
Member

timdorr commented Feb 1, 2016

Right now we don't handle resets in Dev Tools. I'm pretty sure we can listen to Dev Tool's RESET actions, but that's the crux of the issue.

@timdorr
Copy link
Member

timdorr commented Feb 1, 2016

Also, are you running the master version or an older release? I only fixed the initialState stuff yesterday and haven't released that yet.

@mjrussell
Copy link

Also even if the resets were working, I think you'd get burned in this case because the location action is a POP - #164

@awinogradov
Copy link
Author

@timdorr it doesn't work if I use the source from the master or from the last release. @mjrussell it doesn't work if hack action method to PUSH or REPLACE ;(

@timdorr
Copy link
Member

timdorr commented Feb 2, 2016

It shouldn't. I just wanted to make sure you're talking about 2.0.

@awinogradov
Copy link
Author

Ok, how I can solve this? Do you have any idea?

@timdorr
Copy link
Member

timdorr commented Feb 2, 2016

We have to build support for the dev tools actions. We'll get to that soon, I'm sure.

@awinogradov
Copy link
Author

Good, but it's not about dev tools. Maybe you know the different way to setup location on app start?

@timdorr
Copy link
Member

timdorr commented Feb 2, 2016

Your browser or server request determines that. The next version will set the initial location in state properly. But it's a constant. You can't open a location and say you're somewhere else. If you need to do an immediate redirect, you can do that in your app code somewhere.

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

Right now we don't handle resets in Dev Tools. I'm pretty sure we can listen to Dev Tool's RESET actions

That’s not how DevTools work. They are not meant to expose their internal actions to any other thing.

When you press Reset in DevTools, from the app’s point of view, subscriptions fire, and getState() returns the initial state again. So Reset should be handled by the existing listenForReplays() just fine.

Unfortunately I can’t tell whether the author is using listenForReplays() because no source code was provided.

I’m also not sure whether listenForReplays() reconciles the browser URL with the current state of the store. (IMO if it doesn’t, it should, and that is a valid feature request.)

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

When you press Reset in DevTools, from the app’s point of view, subscriptions fire, and getState() returns the initial state again. So Reset should be handled by the existing listenForReplays() just fine.

Oh, I can see now you already know that well: a00acfd.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

I misspoke about the RESET action. Just postulating some ideas without looking at code 😄

I’m also not sure whether listenForReplays() reconciles the browser URL with the current state of the store.

That's pretty much all it does. It looks at the location in state and compares it with the last known location history navigated to. If they differ, it transitionTo's the location from state. Here's where the magic happens: https://github.com/rackt/react-router-redux/blob/master/src/index.js#L94-L98

I'm at a loss for why the RESET doesn't transition, but I haven't played with it much to diagnose. This (along with capturing router state) is on my shortlist of things needing to be fixed in the library right now.

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

I'm at a loss for why the RESET doesn't transition

Currently it kills the first history action and this crashes the app.

screen shot 2016-02-03 at 02 12 32

I see you fixed this in master though. Let me check that!

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

Yeah, that specific error is fixed. Doesn't reset, though. I'm going to see about comparing against initialState, though.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

I'm pretty sure we're running into #164 now.

@timdorr timdorr closed this as completed in 3ae8110 Feb 3, 2016
@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

While 3ae8110 fixed the POP problem I still think we disregard the initial state.
I check by using

import { persistState } from 'redux-devtools'

// ...

export default function configureStore(initialState) {
  const store = createStore(
    rootReducer,
    initialState,
    compose(
      applyMiddleware(thunk, api, reduxRouterMiddleware, createLogger()),
      DevTools.instrument(),
      persistState('lol')
    )
  )

With persistState(), I'd expect the initial state.routing to be synced to the URL bar on launch as the source of truth. (As with listenForReplays() state acts as source of truth in the absence of user actions.)

@gaearon gaearon reopened this Feb 3, 2016
@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

I see parts to fixing this:

  1. We should reconcile the URL bar the first time too, and not wait until first change.
  middleware.listenForReplays =
    (store, selectLocationState = SELECT_LOCATION) => {
      const getLocationState = () => selectLocationState(store.getState())
      const initialLocation = getLocationState()

      const reconcileLocationWithState = () => {
        const location = getLocationState()

        // If we're resetting to the beginning, use the saved initial value. We
        // need to dispatch a new action at this point to populate the store
        // appropriately.
        if (!location) {
          history.transitionTo(initialLocation)
          return
        }

        // Otherwise, if we need to update the history location, do so without
        // dispatching a new action, as we're just bringing history in sync
        // with the store.
        if (location.key !== currentKey) {
          syncing = true
          history.transitionTo(location)
          syncing = false
        }
      }

      unsubscribeStore = store.subscribe(reconcileLocationWithState)
      reconcileLocationWithState() // <------------------- this is important
    }
  1. We need to somehow prevent the initial dispatch in middleware initialization from happening when initial state is specified

???

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

What if we move the initial dispatch into a separate method? a la

const disconnect = middleware.connect()

Then we could change listenForReplays into something like connect({ twoWay: true }) and we'd know whether to dispatch the initial action based on whether reconcileLocationWithState (from previous) comment already tried to transition or not.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

Thanks for looking into this. I'm not a Dev Tools user, so it helps to have a second set of eyes with more experience.

I agree with that behavior. I got tripped up a bit by history.listen, which fires when attached, unlike store subscribers. And ignoring the UPDATE_LOCATION action to initialState is definitely a good thing, as right now you get an action on your stack when loading the page up or after a reset or revert.

something like connect({ twoWay: true })

I'm not a fan of configuration over convention. I'd rather have the opt-in be a function call to enable the behavior.

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

Let me send a PR and we’ll discuss there :-)

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

Let’s discuss in #255.

@taion
Copy link
Member

taion commented Feb 3, 2016

BTW, if reset support actually is broken, we should remove or amend this test case - https://github.com/rackt/react-router-redux/blob/b6a9c1bb51521bf6f5f416de394658d586683d9f/test/createTests.js#L218-L235.

@webmasterkai
Copy link
Contributor

To be clear, this is fixed when #259 lands. So far my tests with #262 work as expected.

@timdorr timdorr added the bug label Feb 17, 2016
@timdorr timdorr closed this as completed Feb 17, 2016
@awinogradov
Copy link
Author

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
6 participants