Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] Allow management of multiple routers via react-router-redux #5663

Closed
e-e-e opened this issue Oct 27, 2017 · 10 comments
Closed

Comments

@e-e-e
Copy link

e-e-e commented Oct 27, 2017

Hi,

Firstly thanks for all your work maintaining react router. It's a core part of nearly all the react projects I have worked on, and cannot imagine developing without it.

I have recently encountered an interesting case where we need to manage two routers, one in memory and the other linked to url history. We wanted to link both of these routers to our redux store, but it appears that react-router-redux only currently works with a single router instance.

Perhaps optional name-spacing could be added to allow users to set up additional routers within redux and dispatch history actions that target only specific routers via namespace.

This PR #5664 is an example of what I am thinking could enable this functionality without modifying the current API interface for single routers.

It would be great to hear any thoughts on how to best achieve this.

@e-e-e
Copy link
Author

e-e-e commented Oct 27, 2017

Hey @timdorr, just to continue the conversation here #5664 (comment). Sorry if my communication of the issue was not super clear, but your proposed solutions of using the same history object will not solve our problem.

I know that it is not a common case to need multiple routers - but it is a genuine real case for the app that my company has been developing. For a long time we have managed a separate router-like component for handing complex sequences of animated screens. We don’t need or want these sequences to be stored in the url history of obvious reasons like maintaining expected behaviour of the back button, not polluting the url with unneeded data, etc. This has meant that we developed our own, initially quite crude, routing logic and managed it in redux through our own actions (a another solution that you suggested). But as complexity of the desired behaviour grew, we realised that essentially we were rebuilding react-router - not a smart thing to be doing when we are already using react-router and your project has already solved the problem.

However we immediate encountered the limitation that at the moment having two instances of history, one in memory and one that is linked to the url route, is not currently possible. There are two real problems with the current implementation.

  1. The way that location is stored in the reducer means that the two history objects overwrite each other, meaning that redux state is no longer a true stateful reflection of the app.

  2. The router middleware only allows single history. And if you declare multiple the history actions only effect the first middleware declared as it does not pass actions to next.

These seem like an unnecessary limitations - that can be solved with no impact on the simple API - but am I correct to assume by your response that react-router-redux is not willing to address the use case of multiple instances of history at all?

@timdorr
Copy link
Member

timdorr commented Oct 28, 2017

Then don't consult Redux for this. Ask the history instance directly. You don't need to integrate Redux into the router. This gains you no benefits.

@e-e-e
Copy link
Author

e-e-e commented Oct 28, 2017

@timdorr I am not sure I understand. How would could a single history instance be used for in memory routing and url routing. Could you clarify?

@timdorr
Copy link
Member

timdorr commented Oct 28, 2017

Use MemoryRouter to use a memory history instance (createMemoryHistory under the hood).

@e-e-e
Copy link
Author

e-e-e commented Oct 28, 2017

Oh, I think I understand your point. But there is benefit to adding redux. By adding redux we can dispatch history actions from any component to modify the history of either router, with the added benefit that this is all transparent in our stores state history. Components displayed by the memory router can trigger actions which redirect main url history and visa-versa.

We could of course manage this all ourselves in the reducer and implement our own actions to modify the memory history instance, but then that would just be rebuilding what you have already built with react-router-redux.

@timdorr
Copy link
Member

timdorr commented Oct 28, 2017

You can do the exact same thing with your history instance. You should make it as easy to access as a Redux store. And luckily, that's already the case. Instead of connect, use withRouter or Route's render prop. Having something that violates the behaviors of Redux outside of your store is probably a good thing.

@e-e-e
Copy link
Author

e-e-e commented Oct 29, 2017

Perhaps I am not communicating our use case clearly, but we have no desire to violate the behaviours of redux, nor to have a core aspect of our application existing outside of the redux store. Which is why I opened this issue and proposed a possible solution.

I also understand that we could inject the memory router into other components, but this would still mean we are unable to keep track of how actions effect the global state of the app. It's not a sound solution.

I understand that in the majority of use cases, routing is linked to URL history but there are other valid instances where routing logic can be necessary. But just to clarify - react-router-redux has no intention of supporting multiple routers regardless of implementation details?

@e-e-e
Copy link
Author

e-e-e commented Oct 31, 2017

@timdorr I have been thinking about your replies over the last few days and I can't seem to get my head around your resistance to accomodating multiple routers. I can understand having a discussion around implementation details, but not your outright rejection. All the solutions you propose are about avoiding redux, but then you obviously understand the benefits of including router history in redux otherwise you would not have authored react-router-redux in the first place.

I also understand desiring simplicity in code, but at the moment the structure of react-router-redux is prohibitive to extension, which means simplicity comes at the cost of allowing complex usages to be built on top. The way the code is structured means that we cannot simply write a plugin that wraps this library with added functionality. For now, as you seem to be totally unwilling to entertain the idea of any other use case than the simplest, I have published our fork - https://www.npmjs.com/package/react-router-redux-multi. If other people encounter this issue, hopefully they will find this useful.

Please let us know in the future if you decide to consider multiple routers - ideally we don't want to need our fork at all.

@timdorr
Copy link
Member

timdorr commented Oct 31, 2017

One of the most important skills of an OSS maintainer is the ability to say "no". It may seem like I'm being stubborn, but accommodating more and more edge cases makes the codebase more complex and more likely to break down. In addition, RRR is a very tricky library to get right (and 5.0 is not working 100% correctly at the moment), namely around state synchronization and timing of state updates.

Linking together Redux and React Router should only be done for development purposes. Trying to maintain perfect sync is simply not possible because a history instance does not behave like a Redux store. There are many cases where you can break the abstraction. For instance, navigating to something that renders a <Redirect> will cause sync issues. Depending on where you are in the tree, you may come before or after that history transition, and will be out of sync with Redux state, resulting in you taking incorrect actions in your code. It's not your fault, per se, it's just that you're looking at the wrong source of truth.

For that reason, that's why I recommend looking directly at your history instance. Within the scope of a <Router>, you can access it via context (or withRouter as a convenience) and be assured that it is always correct and in sync with where that history says the location is. You can also create your own central history instance and pass that around either on context or just have it exist as a singleton module. If you want to use it to drive any <Router>s, just pass it as a prop.

Again, syncing the history with Redux doesn't gain you anything. It makes it much more likely for you to run into errors (especially hard-to-track ones), and makes your codebase more complex. Please don't do it. Just talk to a history instance directly and you'll be OK.

@e-e-e
Copy link
Author

e-e-e commented Nov 1, 2017

Thanks for the detailed reply. I really do appreciate the work you are doing and have done on react-router-redux.

I am aware of the limitations already (its clearly flagged in the comments). Luckily we don't rely on location being in perfect sync with the redux store. Your arguments are really sound for not using redux for a single router, as it really is not necessary apart from debugging and the benefits in production mentioned here reactjs/react-router-redux#257 (comment). We use it for analytics in production. But this does make me wonder why you are even maintaining this project, and why it is referenced from the react-router docs, if you so strongly believe people should not be using it?

So yes I agree - people should not rely on react-router-redux for a single router. But with multiple routers it actually does again make sense for redux to be used.

For example, if you have two routers, as we do, and you want communication between them. withRouter and context.router.history are both not an option as they only allow you access to the history passed down from the router hierarchy. If you want to signal change in a memoryRouter from a hashRouter, or visa-versa you need another means of communication. This could be a Saga, or middleware or any number of options. But we suggested this change since react-router-redux has already established the pattern of using actionCreators for manipulating history, and it did not make sense to build another solution in addition to one already present in our codebase.

Now rather than being an active contributor and test user of your project, we will continue to adapt our fork to suit our needs. It seems a waste, but necessary for our use case.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
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

2 participants