Use middleware to synchronize store to history

[taion]

Per #129 (comment).

Fixes #108.

The idea is basically - keep unidirectional data flow for updates, most of the time. This works in most cases, but it breaks replay, since the actions in some sense aren't pure. That's fine, though - we can restrict the store observer to only handle the replay case (and potentially not even use it otherwise).

[taion]

This actually fixes #140 as well.

[taion]

I believe this fixes #122, too.

[taion]

Probably also #113, since I think using location.key avoids the edge cases you get from an incrementing changeId.

[taion]

Note that you cannot in general directly update the store based on just the action - it's possible for e.g. a listenBefore hook to block the transition, and that would then take the store out-of-sync with the browser.

[tomatau]

Just tried it out and it does fix #108 with a nice implementation. Feels more in line with redux ideals as a simple middleware.

+1

I think it would be worth updating usage and example in this PR for clarity.

[ezekielchentnik]

@taion I believe location.key is not unique, it's random - different every time .push/replace are called, regardless if same path, state. not sure if that matters...

[taion]

I know exactly what location.key is, thanks.

[taion]

@tomatau I see you beat me to it. The one wart is that you need middleware.syncHistoryToStore per https://github.com/rackt/redux-simple-router/pull/141/files#diff-413f2628e9c93c46cec6db202ddb8ea4R159 if you want the Redux DevTools to work (but in normal operation that bit is a no-op).

Wanted to get some feedback from @jlongster and @kjbekkelund as to whether this approach seems workable. I don't use Redux at all myself, but it seems like preserving unidirectional data flow via a transducer just gets rid of all the cycle management headaches (though it requires that extra line of boilerplate for DevTool support).

This is halfway a PoC just to illustrate what's involved though. Ideally, for a breaking change, it'd be best to make the user-dispatched actions take location descriptors (and correspondingly probably just have a different type), while having the reducer operate on actual locations from history.

[kimjoar]

@taion Very interesting! I'm quite busy until early January, but hopefully @jlongster has got some time to review and test. Otherwise I'll test as soon as I've got some spare hours :)

[tomatau]

The one wart is that you need middleware.syncHistoryToStore per https://github.com/rackt/redux-simple-router/pull/141/files#diff-413f2628e9c93c46cec6db202ddb8ea4R159 if you want the Redux DevTools to work (but in normal operation that bit is a no-op).

Testing this out seems buggy as it's dispatching new updatePath actions when rolling back history... which is very bad and counter-intuitive for debugging. Not adding this line seems like a nicer experience, to just go without reversing through the path history from DevTools rollbacks.

I appreciate that this is mainly a PoC, but I'm very keen to see one of these new implementations get merged and fix up #108 ASAP :) So put the word out for any work to get movement and I'm happy to contribute.

[taion]

It's tricky. Suppose you want to reverse the path history.

This necessarily has to have a side effect. I think the right way to model this on the history side is with a push, but that will create a new location with a new location.key.

If I don't dispatch the extra "update path" action, then the key for the location would be out of sync between history and store.

It's possible to directly use history.transitionTo and keep the same key, but that means we'd want to preserve the action involved, and it'd seem odd to me to issue a REPLACE or a POP to the history rather than a PUSH here.

[taion]

Well, maybe using history.transitionTo would be all right.

[jlongster]

I've been resisting to forcing a middleware on the user, as I think a middleware should only be for converting one action into multiple actions, but I haven't had time to look closely at this. I will do so this week though.

Also, long-term you and other people who work on react-router will probably be involved in maintaining this so you have a greater say in how this evolves than other PRs in my opinion.

[taion]

I think the problem with round-tripping this through the store is that it breaks unidirectional data flow. If you do this before it hits the store in the first place, and only use the store for restoring location on replay, you completely exclude issues like #108; otherwise it just feels like a bunch of work to avoid cascading updates, which Flux-like architectures should inherently exclude.

[jlongster]

Yeah, there are benefits to it, but I'd say that #129 is still not that much work and it avoids other problems that this may introduce (there are so many little edge cases with the devtools), without the need for the user to install a middleware. I'm open to it! Just need to look at it more.

[taion]

Let me flesh this out with the changes we mentioned earlier.

[taion]

Updated the PR. This now closes #95 as well. It's a more significant API change, though.

[taion]

@tomatau

This should fix your #141 (comment), except when completely clearing all the actions.

The idea is that this adds the same asymmetry between providing a location descriptor to the action creators, while populating the store with fully fleshed-out location objects.

[jlongster]

@taion I've always wanted to avoid forcing the user to install a middleware. However, I don't really care that much. Really, the two core ideas that need to exist for me to be happy are: only dumb JS objects stored in the app state, and the user uses react-router's API directly in routing components (and wherever else they. As long as that's the case, I don't really care how it works.

You have a much better understanding of how more complex stuff like async routing works also.

If we pursue this path, are you going to continually be involved here? I foresee myself being less involved over time, as I was really hoping to kick off this new direction (and I'm happy it succeeded). I'd love it if you and other react-router folks eventually took over maintenance, but I don't know how burdened you all are with that already.

[jlongster]

I'm assuming this location object is now the fully parsed location? Can we make it so you can also pass just a string, and if you do that we just convert it into { pathname: location }?

[jlongster]

I know that makes the API slightly asymmetrical (the state would be different than what you passed in), but now that we can always store the same shape of object in the state I think it's ok.

[taion]

It's already supported. See e.g. https://github.com/rackt/redux-simple-router/pull/141/files#diff-413f2628e9c93c46cec6db202ddb8ea4R338.

[taion]

Oops - misread. I'm actually not exporting updateLocation at all. The transition actions do take strings, though.

[jlongster]

oh, great, looks like that's a natively-supported thing in react-router.

[taion]

Yes – from the TRANSITION actions. This is just for receiving the update from history for the reducer.

[taion]

This doesn't touch on async routing at all – and it can't, without a much larger re-write. The routing data is only exposed to the children of <Router> right now. By contrast, as you've discussed elsewhere, the library as-is only integrates into history. It's difficult; I can see some ways to make it work, but since I don't actually use Redux, I'd have no clue whether the API would be satisfactory to users or not.

As for my time involvement, I don't know – I don't use Redux, so this is just my vague impressionistic idea of how a Redux integration might look from what I've seen on paper.

Maybe to break down the pieces here a little bit, though, there are a few things here worth highlighting:

• I feel like the primary purpose of an integration like this is to sync history and store in normal use. Intercepting everything in a middleware gives you that, because the data flow is just either action -> history -> store, or history -> store; you don't have to deal with both an action -> store -> history path and a separate history -> store path.
  • Correspondingly, the dev tools integration to cause side effects on modifying action history is secondary; if the user doesn't want this, he or she doesn't have to use middleware.syncHistoryToStore at all, so it's the same amount of API work – just a matter of installing the middleware instead of syncing the history to the store
• With the new history APIs, the natural point of integration is having the user specify a location descriptor, but putting location objects in the store. Among other things, this is how all the history APIs will work, and it can allow e.g. supporting named routes in a very generic way (history.push({ name: 'foo', params })) and have things just work for a user of this library. This means, though, that the "raw" user actions probably shouldn't hit the reducer without going through history first, since you want locations at the reducer, not location descriptors.
• Using changeId seems quite fragile – you're probably better off using location.key instead, and using history.transitionTo to restore the key after a dev tools action.

[taion]

To elaborate, with location descriptors, you really want to separate the location descriptor the user provides from the location that goes into the store. That means the current approach of feeding the user-supplied action directly into the reducer is unlikely to work well.

You could e.g. have a "last location descriptor action" reducer that a store subscriber listens to, then uses to update history, and you'd still have the same architectural design where all updates to the actual reducer for the locations comes from the history (necessarily so), but I just think a middleware is cleaner (really, more unidirectional) here.

[taion]

Not easily – you'd need something like #138, where Redux actually controls the history that the router uses as its source of truth for the current location, or at least a more restricted version that injects rewinded location states into the router.

I think that takes us back to redux-router-like levels of complexity.

But I think the complexity is just about equivalent with this implementation – you just call into history.transitionTo again, and you end up where you want to be with respect to the router.

Ultimately the issue here is that, since Redux isn't driving the router state, we need to do something impure after dev tools actions to sync the router state back up to what's in the store. I think something like the current approach with an explicit "sync" makes the non-dev-tools code path a lot simpler and more predictable – we just need to be a bit careful with edge case handling on syncing the history after manipulating action history.

Whether that should be a store enhancer or whatever – that's just shuffling around a few lines of code; I think the basic logic right now is essentially correct.

[ezekielchentnik]

@gaearon I'm curious as to when it's appropriate to create a store enhancer. I understand the differences between middleware and store enhancers, but is there some guidance as to when it is a good or bad idea to create a store enhancer vs handling functionality externally?

[tomatau]

@taion & @gaearon

Working with syncHistoryToStore is still not intuitive:

• when rolling back, the app state rolls as expected but the path doesn't change (for me)
• if actions are tied to routes, new actions can be dispatched when working with the dev tools - this doesn't align with the idea of "replaying state".

So, I still feel that syncHistoryToStore should be removed from redux-simple-router. Both the ideas of making a new "SimpleRouterHistoryMonitor" or another middleware for syncHistoryToStore seem sensible.

@taion - the internal complexity of redux-router was nowhere near as big a turn off as the complexity of initial setup for consumers. Redux-simple-router will never reach the same level of API complexity, so I think we shouldn't be putting too much weight on internal complexity when deciding an implementation.

Surely, the aim should be for the most well integrated solution, and so far, from the different approaches I've tested out, having redux drive the history updates using a history wrapper was the best for that... On the other hand, this middleware approach feels the most "reduxy" and least imposing. So for me, both of these approaches hold similar weight.

I'd quite like to see the "redux drive history" approach to be explored further, as it seems it was shot down prematurely due to worries of internal complexity, where really, it was doing quite good job of keeping state and history in sync. History methods just worked. DevTools just worked.

If now is the time to decide how this library will be implemented, it would be worth giving all the solutions a good kicking around and then we can all have more confidence that the best solution was chosen.

[jlongster]

@tomatau If rolling back actions does not change the URL, please give detailed steps to reproduce. When I last tried this, it worked for me. And the whole point of transitionTo inside the store subscriber was to change the URL without dispatching actions.

I think we definitely have room to play around with things, I'm more open to things like middleware/store enhancers now and we aren't near the complexity that redux router was. It had some good ideas though. I do think we should avoid wrapping history, etc. We can make everything work beautifully without too much work, but getting devtools to work is the most complicated, and from what I've heard redux-router didn't work great with it as well.

FWIW I'd release a version that doesn't sync the URL back first. I'm not convinced it's that useful to perform side effect in this case. Then we can see if there's enough demand for it and consider different options.

If by syncing URL you mean to rerender the route when actions are rolled back, that's an often-requested feature and many of the bugs that come in are from people using devtools and expected it to "just work".

[mjrussell]

@tomatau @jlongster the path not changing is probably due to #164, I was playing with it a lot after spending some time upgrading/slimming down redux-router (following React-Router's v2) and running across similar history replace issues and was curious how redux-simple-router was implementing them.

It only happens when hitting the POP's (in this case resetting an action that directly follows a POP) because it cannot correctly replay the POP event when it is adding to the store.

[mjrussell]

I tried writing a breaking test for it, but trigger a POP event is actually pretty hard unless you have full control over the browser and the tests are currently designed to run without knowledge of the browser (which works great in every other case)

[taion]

@mjrussell

Just use createMemoryHistory and call goBack or whatever. That said, I just don't know how to deal with this right now.

The problem with the "Redux drives history" approach is that, right now, history doesn't even drive history – it's just a proxy for the browser history API.

I don't think in a production use case, you want to deal with trying to completely hijack the browser history API. That API isn't designed for this, and it doesn't really play nicely with this sort of control.

I'm fine with complexity - we actually have a ton of complexity in the RR and history RCs for handling backward compat. What I'm not okay with is flakiness, and I think hijacking the browser history APIs is going to lead to unacceptable flakiness for production, user-facing pages.

That said, "Redux drives history" might be a decent approach for playing nicely with dev tools...

[tomatau]

@tomatau If rolling back actions does not change the URL, please give detailed steps to reproduce.

@jlongster I have a repo branch that you can see the behaviour in action. here

This is the syncHistoryToStore call on the client https://github.com/tomatau/breko-hub/blob/rSrTest/src/app/entry.js#L23

And here is where the middleware is added on the client https://github.com/tomatau/breko-hub/blob/rSrTest/src/app/state/middleware.js#L17

It will sporadically update the path when routes are updated through the DevTools, but 9/10 times, the path doesn't update. Also, when the path does update via the DevTools, it's coming from a new action, which it shouldn't be.

This is on "redux-simple-router": "^2.0.1"

[taion]

Can you step through ~https://github.com/rackt/redux-simple-router/blob/master/src/index.js#L84-L102 to see what's happening? Unless you're completely wiping history (in which case we need to push a new action to get the store into the correct state), you should hit the forward branch there, and as long as syncing is set, the history shouldn't update the store.

[jlongster]

Do you all mind looking through the docs here: https://github.com/rackt/redux-simple-router/blob/master/README.md particularly the API section? Is everything accurate?

I will push out 2.0.0 today or tomorrow.

[taion]

• The middleware has an unsubscribe method that unsubscribes everything
• The reducer is only necessary if you're using syncHistoryToStore (or if you want to, you know, actually observe the location); the middleware itself will work without it otherwise
• goForward and goBack have no args, while go just takes a n (signed int)

[timdorr]

• The middleware has an unsubscribe method that unsubscribes everything

Fixed!

• The reducer is only necessary if you're using syncHistoryToStore (or if you want to, you know, actually observe the location); the middleware itself will work without it otherwise

Correct, but I wouldn't want to lead beginners down the path of thinking it's optional and then having them be confused when state.routing is empty.

• goForward and goBack have no args, while go just takes a n (signed int)

Fixed!

[billyjanitsch]

Sorry that I'm commenting after v2 has shipped, but for what it's worth, I think this new API has bad implications for separation of concerns, and is a lot more complicated to setup.

The example given in the tutorial looks reasonable enough until you realize that a typical Redux setup will be doing much more than the following two lines:

const createStoreWithMiddleware = applyMiddleware(reduxRouterMiddleware)(createStore)
const store = createStoreWithMiddleware(reducer)

Think additional middleware, store enhancers, reducer updating, etc. A very common pattern is to abstract away this setup into a separate function which returns an enhanced createStore function, parameterized according to the Redux spec. This is a really clean abstraction.

However, using redux-simple-router@2, createStore now requires access to the history singleton to pass to syncHistory. createStore can't initialize it because it needs to be passed around elsewhere (e.g. to react-router), but you can't further parameterize createStore without breaking its natural signature (preventing further store enhancement). It also must now hook onto its own store creation so that listenForReplays can be called on the store once it's created.

At that point, if you're already entangling all of this history logic in your store creation -- left as boilerplate for the user -- why isn't redux-simple-router just a store enhancer? This would eliminate the need for listenForReplays and allow createStore to be enhanced by redux-simple-router at the top level. Your history logic stays separate, there's less boilerplace, you don't need to break createStore, and you get the same behavior.

[timdorr]

react-router embraces singleton histories for version 2.0.0 (coming out soon!): https://github.com/rackt/react-router/blob/master/upgrade-guides/v2.0.0.md#history-singletons-provided

You can similarly define your history in a singleton manner, making passing it around a lot easier and not requiring you to include it as an argument to your createStore wrapper.

[mjrussell]

@billyjanitsch I agree that this new structure does typically require you pass the history both to your createStore and your Router, but how would a store enhancer in this case improve that?

As I see it the store enhancer would still need the history access and therefore you just moved the call from inside applyMiddleware to the compose but it still lies in the same code. Unless of course you pass down the store enhancer to this configureStore function but thats not really any better.

Maybe some psuedo js of how it would be used would make it more clear, perhaps Im just missing something.

(just saw @timdorr comment but thought I'd still ask the question)

[billyjanitsch]

@timdorr

react-router embraces singleton histories for version 2.0.0 (coming out soon!)

Ah, thanks for the link, I hadn't seen this! That's a huge improvement, and solves most of my wall of text above aside from listenForReplays. :)

@mjrussell

Sorry, I was unclear. The idea is that your store enhancers should work separately from each other so that you can (conditionally) compose them however you'd like. When listenForReplays (which is essentially a user-end store enhancer) depends on some parameter of applyMiddleware -- even with access to the history singleton -- it breaks this composability.

Perhaps I want to abstract my middleware:

import {createStore, applyMiddleware} from 'redux'
import {syncHistory} from 'redux-simple-router'
import {browserHistory} from 'redux-router' // thanks @timdorr :)
import middleware from './myMiddleware'

const enhanceStore = compose(
  syncHistory(browserHistory),
  applyMiddleware(...middleware)
)

export default enhanceStore(createStore)

Or abstract most of my store enhancers:

import createStore from './myCreateStore' // already enhanced by me
import {createHistory} from 'history' // stuck on react-router@1 :(
import myReducer from './myReducer'

// with react-router@2, these lines wouldn't be necessary
const history = createHistory()
const finalCreateStore = syncHistory(history)(createStore)

const store = finalCreateStore(reducer)

render(<App store={store} history={history} />, container)

It's not any particular case; it's the general principle of keeping middleware and store enhancers abstractable. I believe that store enhancers were in fact designed to keep middleware from having to do this sort of thing.

[mjrussell]

@billyjanitsch Thanks I see where you are going, especially if you split your enhancer into a separate file then you do benefit from some decoupling in that case because you can do the sync to store call inside the enhancer.

[taion]

The main constraints here are:

1. The basic middleware is just like any other middleware (other than needing the history, which it can get from a singleton exported by some module), and is very simple
2. Without a special history per Make redux drive history? #138, you need something else after the dev tools to pick up on stuff devtools does
3. Ideally, the store enhancers from (2) and (1) can communicate to each other, to prevent (1) from generating new Redux actions when restoring history location, unless we go with Make redux drive history? #138

I'd largely be happy with whatever – the main goal here was just to move to an implementation in (1) that naturally excludes the possibility of updating history twice, through the previous syncing method.

[taion]

And I'd add that you'd want to probably completely discard the replay logic when not in development.

[tomatau]

@taion, apologies for the delay.

Can you step through ~https://github.com/rackt/redux-simple-router/blob/master/src/index.js#L84-L102 to see what's happening? Unless you're completely wiping history (in which case we need to push a new action to get the store into the correct state), you should hit the forward branch there, and as long as syncing is set, the history shouldn't update the store.

A POP is being passed to history.transitionTo with the correct previous path etc... This seems to just get swallowed. After which, history.listen gets invoked correctly, and the store is returning the correct location.key -- but the path hasn't updated in the browser still.

NB: It's a little bit painful to debug with the DevTools or whatever monitor, repeatedly emitting store updates.

[taion]

@tomatau I believe that's #164, then? Where do you see the additional store updates come from?

[tomatau]

Looks very related to #164

When I say store updates -- I'm talking about from the DevTools or LogMonitor itself, it seems to fire some sort of tick invoking the store.subscribe handler. Only a pain point for triggering the debugger.

[pdf]

After this refactor, how are we supposed to access the router state in an immutable store?

[mjrussell]

@pdf maybe you are looking for this? - #218

[pdf]

Indeed, thanks @mjrussell