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

Make redux drive history? #138

Closed
naw opened this issue Dec 25, 2015 · 18 comments
Closed

Make redux drive history? #138

naw opened this issue Dec 25, 2015 · 18 comments

Comments

@naw
Copy link

naw commented Dec 25, 2015

The idea behind redux-simple-router is to keep history in sync between redux and the router history.

In my opinion, it's preferable for redux to be the source of truth for the router, rather than history to be the source of truth that is shared by the router and redux.

My idea is this:

  1. Let the browser history tell redux what to do (e.g. back, forward, initial page load, etc.)
  2. Let links, buttons, etc. tell redux what to do (i.e. not tell the router what to do).
  3. Finally, let redux tell the router what to do.

I have a basic draft of this here:

naw@5d6c893 (update: newer draft here: naw@a47914f)

The basic implementation is:

1 Give react-router a memoryHistory object instead of the real browserHistory object.
2. The only thing that writes to memoryHistory is a redux store subscriber.
3. wrap the memoryHistory object we give to react-router so that we can intercept push calls so that they call a changeLocation action creator rather than writing to memoryHistory directly.
4. Separately, listen to a real browserHistory object, and call the changeLocation action creator from the history listener.

Also, I'm using a slightly different approach for preventing cycles, so I believe the locationsAreEqual complexity is unnecessary.

I've probably missed some edge cases (or perhaps I've missed something larger than that?), but I wanted to share the basic idea and get some feedback.

@kimjoar
Copy link
Collaborator

kimjoar commented Dec 25, 2015

Hi @naw, interesting approach. It's late, so I don't have to look much at this right now, so just a few questions that jumps to mind:

  • Is browserHistory.listen guaranteed to be synchronously called?
  • Have you ran it through all the tests? Lots of cases with regards to devtools etc
  • Does this override push in memoryHistory? Seems "hacky" (though without looking much at this)

@naw
Copy link
Author

naw commented Dec 26, 2015

@kjbekkelund

Thank you for your initial feedback.

The original commit I shared was a completely separate implementation providing a basic proof of concept by serving as a drop-in replacement within the example app. It didn't actually rewrite the existing code, so none of the tests would apply.

Therefore, I have refactored my original implementation so that it is now an actual modification to the redux-simple-router code, keeping things as similar as possible (such as router names, action creator names, etc.) so that the tests can run with little modifications.

The new refactoring is here:

naw@a47914f

Now to address your questions, and provide some additional clarification:

All of the tests pass (most without any modification). I believe the only modifications were to accommodate the removal of avoidRouterUpdate, and to handle a change in how you get your hands on the unsubscribe function. In particular the devTools stuff works fine.

The main goal is to make redux the source of truth. The only interface available for react-router is the history parameter that Router accepts. Therefore, in order to let redux control the router, and also force <Link> to go through redux, we have to give react-router a "fake" history object that we can control. By fake, I just mean an object that exposes the API that react-router expects for reads and writes to location. This is just duck-typing. The easiest way to implement this was a thin wrapper around memoryHistory, although it's conceivable to create such an object from scratch. Wrapping objects like this is very similar to what useRoutes in react-router does and what useQueries in history does. I personally wouldn't call it a hack, but I can see why you might feel that way. Sometimes there is a fine line between elegance and hack.

I believe this overall implementation yields some simplicity:

  1. avoidRouterUpdate is no longer necessary.
  2. areLocationsEqual is no longer necessary.
  3. Because of (2) , it seems like supporting query params would be easy to solve (I'm assuming areLocationsEqual was the primary problem, but perhaps I misunderstood Support query params? #95)
  4. It's feasible to let updatePath and replacePath accept a single object --- any object that history.push will accept (e.g. either a location object or a location string.) As a result, you could ditch createPath and just pass location directly through redux.
  5. On the other hand, if you did want to normalize/parse things there is a single entry point where this can be done.

Finally, regarding your question "Is browserHistory.listen guaranteed to be synchronously called?"

Is there a specific scenario you had in mind that would be a problem in this implementation, but not in the existing implementation?

Thanks again.

@taion
Copy link
Member

taion commented Dec 26, 2015

This is effectively the approach that redux-router takes. I haven't really been keeping track of why that project is no longer in rackt while this project is, but:

  • The explicit goal of this project is to let the router handle the state management, because
  • In practice, redux-router ended up being quite complicated and a lot less easy to understand/use.

@taion
Copy link
Member

taion commented Dec 26, 2015

From the README:

Let react-router do all the work

Having Redux manage the state is definitely an interesting approach on paper, but dealing with the edge cases is not easy. I'm not directly involved with redux-simple-router, but I'd personally suggest working with the redux-router maintainers if you're interested in pursuing that approach.

@tomatau
Copy link
Contributor

tomatau commented Dec 26, 2015

This fixes #108 for me, seems like a very clean approach.

+1

@jlongster
Copy link
Member

Personally I see about just as much complexity here (if not more, having to manage multiple histories).

You still have two flags: browserListeningToStore and storeListeningToBrowser. Those are exactly the same as avoidRouterUpdate, except it isn't stored in state, which was done purposefully because it was easy to debug. But check out #129 where we will probably move it into local state just like your 2 flags are.

Additionally, there is a complex flow here: if the user presses back, it's going to trigger an event on the browserHistory instance, but if a user clicks a link, they are going to trigger the memoryHistory instances. Seems very confusing to maintain.

In fact, since everything stills works when pressing the back button, and it's going through browserHistory, your PR does not fully solve the "two sources of truth" problem the way you want it to. I don't see why you would need the wrapper at all. Fact is, there are always going to be two places that router state updates from, which is why you still need flags.

Initially I responded similarly as @taion, because if you want only one place where all router changes come through you effectively have to write your own routing API, which is what redux-router does. And APIs are hard, and react-router has already done a ton of work on that front, so I think it's best to just leverage that.

But I do like your approach (I'm assuming the memoryHistory wrapper does solve certain problems). You found a way to still allow users to use react-router directly but sort of monkey-patch the system to hook into react-router at a deeper level. Well done. This is actually still much different than redux-router because you aren't writing your own API.

I think I'd be interested exploring this approach except for one thing: they are working on react-router 2.0 and the new APIs will allow us to implement something very similar to this, but less confusing. Basically, these sort of history wrappers will be first-class and we won't even have to use history.listen anymore (if I understand correctly). I don't fully understand those changes yet, but I've been told it will be easier (@taion works on react-router). I'd rather get #129 merged in and then work on an experimental version that works with react-router 2.0, and I'd love your help with that.

@taion
Copy link
Member

taion commented Dec 27, 2015

We actually ended up in a very different place with 2.0 - we've explicitly taken out the ability to listen to router state, and made it clear that if you want to listen to router state, you want a component under the <Router>. I don't really think there's anything new for redux-simple-router in the new API. We mostly just cleaned things up.

A few things to think about with this API:

  1. How do you make transition hooks for preventing transitions work? Those will happen from the browser history, but you need to deal with them from the proxy history.
  2. How do you wrap different actual histories? e.g. also supporting hashHistory (which will need a different createHref), but also e.g. basename and query string options...

Mostly reduces down to edge cases that I think might make this approach messier on net.

@naw
Copy link
Author

naw commented Dec 27, 2015

@jlongster

Thanks for your response. I'm sure you already know most of this, but for sake of clarity, let me try to be a little more verbose, and please forgive me if I'm belaboring the point.

There are three pieces at play here: router, browserHistory, and redux.

I'm not saying this is a two sources of truth vs. one source of truth issue -- it's a question of which single piece is going to be the source of truth.

In the current implementation, browserHistory serves as the single source of truth. It takes input from <Link>, Back/Forward, and redux, and then sends that update out to redux (if necessary) and the router (always).

In my proposal, redux serves as the single source of truth. It takes input from <Link>, Back/Forward, and browserHistory, and then sends that update out to browserHistory (if necessary), and the router (always).

If you compare the above two paragraphs, you'll see that the overall "shape" of data flow is identical, but redux takes the place of browserHistory as the source of truth, whereas browserHistory takes a back seat as a UI input (just like clicking a link is a UI input). What I'm trying to say is my implementation does fully solve the source of truth issue that I was trying to solve, even though browserHistory is both an input and an output. Think of the address bar as being like any other UI control --- when you type something in a form text box, for example, it is both an input and an output of redux --- informing redux to update its state, but also accepting redux state as source of truth for its value.

The premise I'm starting with is that all things being equal, it's better for redux to be the source of truth than for browserHistory to be the source of truth. Starting from that premise, I created an alternative implementation. A side-effect of that implementation is what I perceived as overall simplifications (although of course I'll grant that it's debatable whether my implementation is actually simpler or not).

There are actually two somewhat individual issues here:

First, which object should be the source of truth?

Second, regardless of which object is the source of truth, are there some implementation changes that would increase simplicity?

So I have a a question:

Disregarding complexity for a moment, which object is the preferable source of truth? In other words, assuming we could find a simple implementation to make redux the source of truth, and assuming such an implementation didn't introduce a bunch of edge cases, would you prefer redux to be the source of truth, or conceptually do you feel browserHistory actually should be the source of truth?

If you feel browserHistory should be the single source of truth regardless, then we can definitely just drop the redux as single source of truth aspect of this issue, and I can pursue that goal elsewhere. Meanwhile, we can discuss possible implementation simplifications that keep browserHistory as the source of truth (although perhaps that's redundant in part with #129, as you mentioned).

On the other hand, if you feel that redux should be the source of truth, but there's just too many edge cases to solve in a reasonably simple library, then perhaps this discussion should focus on whether or not my proposed implementation has merit for being a simple solution, or whether it will ultimately lead to the same complexity of redux-router that you were trying to avoid in the first place. As you said, maybe that's difficult to pin down until react-router 2.0 lands.

It seemed to me that my implementation with the two boolean flags and pushing everything through redux not only replaced avoidRouterUpdate (which I'll admit wasn't a huge win), but it also eliminated the need for areLocationsEqual, deepEqual, and possibly helped solve the query parameters issue too (although perhaps I misunderstood the crux of #95). Or, perhaps I'm just misunderstanding the role that areLocationsEqual plays, but the tests do pass without it in my implementation.

Granted, using a proxied history object to interface with react-router introduces its own complexity, so of course there are trade-offs, and those trade-offs are a little subjective.

Thanks for your reply and your patience. If you could give a me sense for how you view the "redux as the source truth" issue as a whole, that would give me a better idea how to proceed.

@naw
Copy link
Author

naw commented Dec 27, 2015

@taion thanks for listing some explicit scenarios you think might cause some trouble with this implementation. It's nice to have some concrete arguments to consider. I'll plan to get back to you on your questions.

@taion
Copy link
Member

taion commented Dec 27, 2015

FWIW, the history object (browserHistory or hashHistory) is not a source of truth. It's a proxy to the browser's own history. We do a little bit of book-keeping (mostly around confirming navigation), but you should think of a history object as much more of an interface to the browser's own history API.

You can't ask a history object e.g. what's at "-3 history locations from the current location" - it doesn't know and can't tell you.

Why do it this way? Because it completely rules out edge cases where the history that we track diverges from the history that the browser knows. And it means that if our user does something like a history.goBack() that actually takes them off the page, we don't need to do anything special - it just works because the browser handles it.

@naw
Copy link
Author

naw commented Dec 27, 2015

When I call browserHistory the source of truth, all I mean is that when react-router asks the question, "What is the current location, so that I can decide what component-tree to display?", browserHistory is the object that holds the answer. I understand it's just a proxy for the browser itself, so in that sense, the browser itself is the source of truth, and you can equate browserHistory with browser in my statements. I don't think we actually disagree on this, so perhaps it's just semantics.

Do you see a difference here in what we're saying?

@taion
Copy link
Member

taion commented Dec 27, 2015

That's actually a bit counter to the design intent. The history objects are effectively stateless - you can't ask them for their current location; you can only subscribe or push/replace.

@naw
Copy link
Author

naw commented Dec 27, 2015

Router Question: "What is the current location, so that I can decide what component-tree to display?"

Answer: "Whatever browserHistory last told me the current location is".

So the state is technically in the router, but that state is effectively being controlled by browserHistory, right? And browserHistory is the only thing that changes that piece of state in the router?

So browserHistory is the source of truth for the current location from the perspective of the router?

What am I missing?

@taion
Copy link
Member

taion commented Dec 27, 2015

Ignoring the transition manager for now (which potentially asynchronously does the actual route matching from the location), the difference is that the <Router> behaves like a store, while the history behaves like an emitter of actions (really like a Flux action creator in that sense).

It's semantics (and it's getting late), but typically you want to think of the store-like entity as being the source of truth.

@taion
Copy link
Member

taion commented Jan 14, 2016

Per discussion on #141, I think this might still be a good idea to get things working as well as possible with dev tools, in the case that users want to be able to toggle router-related actions.

I don't think you'd want to use this in production – but it could dramatically simplify things for development.

@jlongster
Copy link
Member

You can toggle router-related actions with the new middleware approach, no?

@taion
Copy link
Member

taion commented Jan 15, 2016

Yes, but it's sort of not great. There's the lack of support for POP transitions, and it's just a little messy IMO.

@gaearon
Copy link
Member

gaearon commented Feb 4, 2016

I made a proof of concept of this and would love to get your opinion:
reduxjs/redux#1362

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

No branches or pull requests

7 participants