search

acdlite / redux-router

Redux bindings for React Router – keep your router state inside your Redux store
MIT License
2.3k stars 200 forks source link

Proposed simplified and breaking changes with React-Router V2 (Discussion) #232

Closed mjrussell closed 8 years ago

mjrussell commented 8 years ago

This is related to and a continuation of #172. I spent some time this weekend getting redux-router up and running with react-router v2 and found myself rewriting logic almost verbatim from react-router into redux-router. I agree with the goals of redux-simple-router in that we should be leveraging react-router for the logic while ensuring that it can be used in a redux friendly way. However, I think based on the comments in #172 (and my own belief) there is a desire to continue this project because of some of the extra functionality it offers.

Therefore, I am proposing a possible middle ground style approach where we push as much logic into react-router and simplify this library while still maintaining some of the core features. This is only really possible with some of the new features made available in Version 2.x of React-Router which is currently in release candidate mode.

I've implemented a proof of concept here which has both the client and server examples up to date and working (the tests still need some updates though). The library contains about half the code of the original while maintaining the core features, although losing a few more advanced capabilities. I'd like to gauge wether or not these things are critical to the community.

Feature breakdown:

Maintains the following:

Removes the following:

I think the getRoutes removal will be the most jarring at first. For example @erikras uses it in his well known example. However I think that the higher order wrapper method for authentication and authorization in routes as shown in @joshgeller 's example works quite well.

Other benefits

Would love to get thoughts from @Scarysize @acdlite @omnidan (and other contributors) as well as current users on this

Scarysize commented 8 years ago

"Far less code, much easier to maintain and bugfix" is music in my ears!

Seriously though, I think this might foster more contributions to the project by simplifying it just enough. It takes a lot of confidence to contribute to the code base in its current state; this might lower the bar.

damassi commented 8 years ago

I'm going to to take your POC for a spin right now

mjrussell commented 8 years ago

@damassi Awesome! Theres a small bug Im working through right now regarding multiple API calls to history (but it wont affect your test spin). Please let me know how you find it. If you have any issues put them here or ping me on Discord

damassi commented 8 years ago

Will do. The app I'm working on is fairly complex so I feel like I'll have a good idea of what's involved once I finish the switch.

damassi commented 8 years ago

Ok, I've run into a few snags that have forced me to reevaluate why exactly I'm using redux-router in the first place, which is simply to snatch location props from the state when really they're now being piped right through via the router directly into props. Looking over my code It would probably benefit to simplify things for the long run rather than rely on a cross-section of functionality that's already available in RR v2. If I had a bit more time I would be able to dig in and see what's up, but I've got.... a whole series of other major updates I need to do over the next week from Babel to redux-form. Oh Javascript!

mjrussell commented 8 years ago

:disappointed: Sorry to hear that. One note is that those props are not being passed beyond the first level (i.e. the child routes) using React-Router.

The goal of this refactoring/new API was to bring the code much more in line to almost verbatim how React-Router works while adding the extra piece to allow for the store to hold the props (with little overhead) and to dispatch route changes via action creators. You might also look at redux-simple-router for a stable "lean" integration if you aren't using many of these features

damassi commented 8 years ago

I'm going to give that a look now and may return to this this evening once I have a better idea of the scope required. Thanks for taking the time to give this library a little attention.

HriBB commented 8 years ago

I tried https://github.com/mjrussell/redux-router/tree/rr-v2 and I managed to get client side working, but not without problems. I use router params in componentDidMount() to load the right data. First problem I had was that state params in my container were different than params from the react-router. Router params were empty on componentDidMount() and it broke down. I fixed this by using redux-router state params instead of router params. Could be a bug on my side.

Will do some more tests later, then on to the server side.

mjrussell commented 8 years ago

@HriBB the rr-v2 branch is a pretty radical approach to reshaping how to sync the store data (by using the render middleware of React-Router). It could have issues and Im definitely concerned you didn't get the routerParams on cDM. You might try the other branch I had which is a very modest upgrade at https://github.com/mjrussell/redux-router/tree/react-router-2.

That being said, it seems like the react-router-redux is moving in the right direction and will be much improved once they finished retooling the API. That definitely seems to the be "community-approved" way, but I know that redux-router will provide some benefits not found in react-router-redux.

If there is enough community support Im happy to try and continue with either path (the modest upgrades or more radical one) and help get a beta version out for those who want to continue with Redux-Router and get on React-Router 2. That being said I think that anyone considering that approach should at least look at react-router-redux at some point to make the best decision.

gaearon commented 8 years ago

Not sure if you have been following but you might be interested in https://github.com/reactjs/redux/pull/1362 and https://github.com/reactjs/react-router-redux/pull/259. While it still doesn’t store the query parameters in the state, it might be interesting to compare approaches.

HriBB commented 8 years ago

Thanks guys for the info. Very helpful. Will try react-router-2 branch tomorrow. I'm happy to use react-router-redux if I can sync router params into store. Can I do that?

HriBB commented 8 years ago

OK so I tried react-router-2 branch and client side works perfectly. Good job. Now on to the server

HriBB commented 8 years ago

Server seems to be working as well. Cool :) Will test it out further during the week and report any bugs.

mjrussell commented 8 years ago

@HriBB glad to hear that the react-router-2 branch is working for you. Does it not have any of those componentDidMount issues you ran into?

react-router-redux does not allow you to sync the route params into the store, they do sync the entire location object though. The params are available as props in all components returned from a <Route>.

Regardless, seems like moving the "modest" upgrade to React-Router 2.0 api into a 2.0-beta release for Redux-Router might be a good idea after further testing @Scarysize.

Scarysize commented 8 years ago

@mjrussell sounds good. I currently have no idea how much this project is still used, though the amount of new issues has declined greatly since the last release (which may point to either just a "stable" version ooor less usage). I need to get an overview about current problems and bugs, then we could come up with a milestone for a next release (if you´re up to it). I guess it would be forth to develop this in a direction which provides some distinct features compared to react-router-redux.

HriBB commented 8 years ago

@mjrussell no, no componentDidMount issues with this branch. It worked out of the box. And I agree about moving into a 2.0-beta.

HriBB commented 8 years ago

npm says 1,662 downloads in the last day 10,883 downloads in the last week 47,399 downloads in the last month

Maybe it 'just works' once you set it up :) I believe that having router params in store is a requirement for some, which only redux-router provides.

catamphetamine commented 8 years ago

@Scarysize I'm using it just because the "simple router" wasn't around when I developed my solution for asynchronous routing. Since rackt repo is a good PR many people are routed to the "simple router" now. I guess that would be clarifying if you wrote in your readme how exactly your router differs from theirs. Not like "it has more features" but "it has feature A and feature B and lacks feature C".

Scarysize commented 8 years ago

@halt-hammerzeit Yep, I definitely want to clarify these things in the future. This would also help newcomers choose a module fit for the needs.

mjrussell commented 8 years ago

@Scarysize I can work on getting README file update (or git wiki page, or whatever you think is best) that describes some of the tradeoffs since I've built several small examples and 2 larger projects with both redux-router and react-router-redux. It does seem like people are still using this library, however I think that many of the newcomers are going directly to react-router-redux due to the warning in the README. My guess is that this project supports more legacy projects and those that have code tied to certain features they dont want to lose.

mjrussell commented 8 years ago

239 is a first pass at some of the differences between the two projects. Feel free to comment, pick apart, or ask for more clarification to the text

mjrussell commented 8 years ago

@Scarysize I think we should close this as done since we updated the README and got a beta out for react-router 2

Scarysize commented 8 years ago

@mjrussell Yes, we should ;-)