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

Clarifying the goals of this project #257

Closed
gaearon opened this issue Feb 3, 2016 · 49 comments
Closed

Clarifying the goals of this project #257

gaearon opened this issue Feb 3, 2016 · 49 comments
Labels

Comments

@gaearon
Copy link
Member

gaearon commented Feb 3, 2016

I think I’m getting a little confused by the goals and the scope of this project. I’d appreciate @jlongster weighing in on this.

I see a few things it does right now:

  • (1) Provides state.routing for other middleware, components, and action creators to read. I believe this is problematic: it doesn’t match the rendered route because of the potential for async transitions in between. This is the reason we warn people not to use state.routing in mapStateToProps() which I believe is confusing, and even if they don’t use it in components, they still might use it in action creators and expect it to correspond to the router’s current state. So I’m not sure whether this is a good feature to keep.
  • (2) Provides an actions-first interface to side effects of history. While this may be convenient, I think the reason people want this is not because actions are the best interface for causing routing changes, but rather because people don’t want to deal with context or pass history around. Many people are also feeling pressured to express everything with Redux primitives (actions) even if the problem at hand is not expressed with them well. Consider that history.pushState() is not much different from fetch() (it’s a side effect) but nobody expects us to provide “fetch action creators”. The real problem here seems to be the perceived difficulty of passing history around or importing it, rather than the need for history updates to be actions.
  • (3) Allows replaying the history changes from DevTools or another action replay mechanism. This is where it gets interesting. It seems to me that the ability to persist routing state together with everything else and re-render the app based on current state rather than what the browser thinks was the original point of this library. This feature seems to explain the need for action creators: you want navigations to be in the log so they can potentially be replayed. However such functionality would only be useful in dev (or otherwise special) environment.

As we evolve (or not evolve) this library, I want to better understand which of these points are actually important to the author, and which are just a byproduct of current user habits and their desire to express everything “the Redux way”.

Essentially, I want to understand what value react-router-redux brings over react-router + history in production mode other than the familiar mental model (“just dispatch actions”), the ability to reach out into state.routing (which can be dangerous and may not reflect the UI but can be convenient for apps without async transitions), and not having to think about how to access history without passing it around.

In no way am I trying to diminish the work being done here. I just want to figure out for myself whether presistence/time-travely part of this library could be a separate dev-only project, and whether what would be left is not essentially a feature request for better documentation and less ceremony when accessing history from action creators and other middleware.

Thank you!

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

For a number of reasons (bundle size, code complexity), I've always thought it would be interesting to isolate the Dev Tools integration into its own thing somehow. Not having to worry about it really simplifies the functions used for production. Unfortunately, it's necessary for the current setup to couple the two things, but a split would probably simplify a lot of the confusion.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

What I’m trying to understand is what exactly is the benefit of the rest of the project other than patching usability holes with making history accessible without explicitly threading it?

@inyono
Copy link

inyono commented Feb 3, 2016

We (@ory-am) are basically only using the action creators and the DevTools. Most of the time, we use react-router features directly.

Consider that history.pushState() is not much different from fetch() (it’s a side effect) but nobody expects us to provide “fetch action creators”.

In fact, we do exactly that. We handle side effects with redux-saga and have action creators that initiate the sagas (and those dispatch some actions themselves).

@mathieudutour
Copy link

Dispatching an action instead of using directly history.pushState() is really important. The action will go through the chain of middlewares, meaning (for example) automatic analytics, etc.

@k2052
Copy link

k2052 commented Feb 3, 2016

At least in my use case it was simply a matter of "knowing where to put things". The second point Provides an actions-first interface to side effects of history was the biggest one for me. Having history manipulation as actions gave me a clear place to put things that manipulated history and a clear way to do it. Dispatching an action to change history was a clear mental model I already understood.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

Dispatching an action instead of using directly history.pushState() is really important. The action will go through the chain of middlewares, meaning (for example) automatic analytics, etc.

Doesn’t this achieve the same goal?

history.listen(location => store.dispatch({ type: 'NAVIGATE', location }))

@taion
Copy link
Member

taion commented Feb 3, 2016

Let's denote those points as (1), (2), (3).

(2) is actually just its own little thing – I'm quite happy with what we have via #141 from my perspective, though you're correct in that this is not strictly tied to the rest of the package.

(1) and (3) are where it gets interesting and messy.

I think something like my proposal in #248 (comment) would get us to (3), but it's still a bit of a hack, and it's not perfect.

I think the first-best solution to (3) is actually to solve (1), by having Redux drive the router, in which case (3) naturally falls out just by virtue of Redux being Redux. While the router state itself is not serializable, we can digest it into just the location, and use match() to deserialize.

This is much more like redux-router, though, but I think a "simple" version of redux-router might be a viable approach. It would clean up (3) a lot.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

What have we learned from Redux Router? What parts of it were complicated, why, and has this changed?

@taion
Copy link
Member

taion commented Feb 3, 2016

cc @mjrussell, @acdlite on the Redux Router question

I don't want to sidetrack this too much, but I think the way this API wants to look is that <Router> is just like <NavigationContainer> from https://github.com/ericvicenti/navigation-rfc, in that it can be subbed out.

I think we can solve the serializability-for-persistence issue with a custom persistState, but I don't know what the other issues that redux-router faced were.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

Got it. Thanks for the information. I’d say let’s not get too far in this thread with implementation thoughts, and rather figure out the focus better. What do we want to solve with this library and why.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

I would say simplicity is a feature, not a requirement of the project, so if we have to get more complex, that is fine. Maybe not redux-router levels of complex, but not redux levels of simple 😉

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

The problem is that without a clearly defined pain we’re solving, it’s hard to focus.
I think this tweet exchange caught me off guard:

(James) Setting up redux/react-redux/react-router/history/react-router-redux/redux-history-routing-react-router-simple/what-is-happeninggggggg

(Me) Don’t set up react-router-redux until you need it. You can start with just history, or history + RR

(James) when would I need it?

This is when I realized many people might be installing this project and setting up all this stuff without actually needing it.

For example, from the same exchange:

(Florent) if you want to access history and don't have access to history prop.

But this is incorrect because you can do

import { browserHistory } from 'react-router'

and call browserHistory.push() just fine!

Similarly, from the exchange in this thread:

Dispatching an action instead of using directly history.pushState() is really important. The action will go through the chain of middlewares, meaning (for example) automatic analytics, etc.

But again, it is a matter of

import { browserHistory } from 'react-router'
browserHistory.subscribe(location => store.dispatch({ type: 'NAVIGATE', location }))

when creating a store if you want it.

So this is why I want to get a better understanding of what the goals of this project are. Because if it’s persistence and DevTools-friendliness, perhaps it’s easier to achieve in other ways, without providing state.routing, action creators, etc.

@taion
Copy link
Member

taion commented Feb 3, 2016

I think that's a point that's worth making either way – in general you're not going to need anything to use React Router with Redux, at least when starting out.

Certainly I've never felt the need to put router stuff in any of my Flux stores or actions.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

import { browserHistory } from 'react-router'

Actually, up until 2.0.0-rc1 (a month ago), you could not. And you still cannot without running non-final packages. You can certainly create your own singleton, but I don't think most people know to do that. Hence, the popularity of this library lies in the limitations of React Router 1.0.

@taion
Copy link
Member

taion commented Feb 3, 2016

That's not that much of a limitation, no?

// history.js
export default createBrowserHistory();

If you want to use browser history, you're doing this anyway – just need to expose it, is all.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

I think what I'm saying is the goals have shifted. What was once a major requirement is now more of a convenience. I noticed a lot of our discussion on Reactiflux was centered around Router 2.0 concepts and internals. I think we should be aware that most people aren't using that version yet, so the necessity of this library is going to diminish.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

@taion Yes, it's easy to do so. But I don't think most people know they can do that. We weren't often giving that kind of guidance to users. Most people treat module imports like functions or classes, not singleton instances, so I don't think that behavior is commonly considered.

Anyways, now that router is advocating their use, one of this library's big advantages will be going away.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

The thing is, once 2.0 is out, we can put anything we think we need to into “Usage with Router” official doc. We can teach people to use singletons (whether exported by RR or manual) there just fine. People treat Redux docs with respect :-). So let’s take this into account. If it’s an education problem, we can fix it. Is it an education problem, or are we missing essential use cases (beyond DevTools and persistence)?

@taion
Copy link
Member

taion commented Feb 3, 2016

Do we advocate its use? We merged remix-run/react-router#2992.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

It's sounding more and more like the version of this lib that best matches with router 2.0 is one that only deals with devtools stuff.

@taion
Copy link
Member

taion commented Feb 3, 2016

I don't understand, though. We merged a PR with what seemed like the rationale that using singleton histories was fragile and undesirable.

It can't be the case that it's not okay to use singleton histories in that context, but is okay in this context.

@timdorr
Copy link
Member

timdorr commented Feb 3, 2016

@taion remix-run/react-router#2991 has some more on the subject. I still think it's weird for presentational components (especially functional stateless components) to reach into the outside world. That LinkButton case should have gotten at it via context, rather than going to the global-ish browserHistory singleton. I'm thinking the singleton is better used outside of React. That's where most people "need" this library right now.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

It may be that built-in action creators are beneficial, but only as a way to dependency-inject the history. This is part of what I’m trying to understand about this project. If this is the case, it’s fine, but we need to understand whether or not this is one of the important conveniences it brings.

@ghost
Copy link

ghost commented Feb 3, 2016

@gaearon Time travel is useful for way more than just developer experience. It is a core feature for production use as well.
Take for example a company who relies on salespeople doing live demos with potential clients and signing contracts. Allowing these salespeople to demo the app with the potential client logged into the app with readonly actions being synced over is incredibly compelling in my experience. Once the demo is complete, they are left logged into the application and control is passed over to them so there is zero transition. Beyond that recording actions of select production users for the UX team to "replay" and analyze is pretty handy for user testing complex workflows/interactions is priceless.

Without this library these types of use cases would be missing a significant portion of the flow.

@taion
Copy link
Member

taion commented Feb 3, 2016

I think the action creators are sort of a "throw them in" thing.

I don't think users want to have to use all of react-router-redux-replay, react-router-redux-state, and react-router-redux-actions. There's some benefit to having a one-stop shop.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

@danmartinez101

Thanks for your input! I definitely understand this use case. I probably shouldn’t have said “dev” and rather just called it “time travel support”. What I mean is that it might be easier to do in a separate project and in a different way, and this got me wondering what exactly is the scope of this project if we remove that.

@gaearon
Copy link
Member Author

gaearon commented Feb 3, 2016

@taion

Definitely! This is why I’m trying to figure out the scope that we really want to support :-) I don’t want to split projects for modularity’s sake—rather, I want to understand which of these features are essential so that we can approach them more holistically and ideally without big hacks or glitches.

For example we may decide “state.routing is not important, but action creators, and full DevTools support is”. This is fine. I just want us to reach some conclusion regarding what is the essence of this project.

@ntkoso
Copy link

ntkoso commented Feb 3, 2016

About browserHistory singleton, singletons and server side = bad idea. Actions and middleware are giving me an easy way to use history without propagating it through context / props on client side.

@ghost
Copy link

ghost commented Feb 3, 2016

I understand more clearly now.
For us, basically all we care about is the ability to record/replay actions including url changes so we can lead/follow a user through the full experience (i.e. route transitions) of the application using actions-over-sockets.

/me fades into the background

@patrickheeney
Copy link

Just a general user here. I installed RR in 1.0 days and using redux router helped fix some API issues, keep up with the redux way and use the dev tools. After the jump to 2.0 I didn't spend much time re-learning, so just upgraded this as well. Like many redux libraries I have discovered them in awesome-redux and thought it could be beneficial to my application without probably understanding if I needed it.

These are some things I learned in this discussion alone, hopefully they make it to the docs somewhere.

history.listen(location => store.dispatch({ type: 'NAVIGATE', location }))

I haven't looked at the entire API surface of RR, Redux, Redux Router and it would be beneficial to know that this could be used a replacement for some functionality of this library. Some libraries have the "do you really need this?" on the README.

function Button(props, context) {
  return <div>{stuff}</div>
}
Button.contextTypes = { stuff: PropTypes.string }

The discussions in remix-run/react-router#2992 and remix-run/react-router#2991 led me to realize you can access context in functional components. I also realized why the history singleton is bad and how to work around it.

I typically only read about the library / API when implementing it and possibly again on a major version update. All of these discussions and document improvement merges I will likely miss until I catch @gaearon next tweets (thank you for your effort, I have learned so much by following you).

@timdorr
Copy link
Member

timdorr commented Feb 4, 2016

@patrickheeney History singletons aren't bad. They simply don't work in the server context, so you have to be using them thoughtfully and not for every single case where you're trying to navigate.

@patrickheeney
Copy link

@timdorr IMO, that is what makes them bad. I will be converting my app to universal at some point and that would have ended up an unpleasant surprise. Following what I learned above I would rather use context in functional components or pass the router change function down as a props. It has the benefit of using less of the API so I won't break if that singleton changes and works on the server.

Based on the discussion above and from a user point of view, I'd like the see the repo evolve into replay functionality and not provide any convenience methods or other API surface area if I can get similar functionality from the RR API directly.

@timdorr
Copy link
Member

timdorr commented Feb 4, 2016

not provide any convenience methods or other API surface area if I can get similar functionality from the RR API directly.

@ryanflorence has made some great, user-friendly API changes for 2.0, so a lot of pain points this library used to resolve are now obsolete. It sounds like we're evolving the focus to more specific needs, like time travel, as router is now significantly better to work with.

@gaearon
Copy link
Member Author

gaearon commented Feb 4, 2016

Please see reduxjs/redux#1362 for something I hacked together based on this discussion. Big thanks to everyone.

@Scarysize
Copy link

From a maintenance/contribution point of view: It´s great to reduce the complexity of a project, though I don´t think this project is in the need of that. If I would come in with the intention to contribute to this project, I can get an overview of the inner workings pretty quickly. What we may have learned from redux-router: Complexity is a pain in the a**, which will make it extra hard for people to contribute.

From a (unexperienced) users point of view: The JS ecosystem right now is pretty complex, so I will appreciate if I don´t have to get three libs to get the full API and functionality of this project, staying up-to-date on all three of them etc. It is okay for me to use maybe only 60% of the API a library offers in production, and I can see the rest as a nice-to-have, if a may run into problems during development. Also, if the project fits well into how I do things with other libs (e.g. using redux actions for every state change including routing), I can easily wrap my head around that.

In general I think extracting the devtools-ability is great from a modularity standpoint, but bad from an usability one. Users will continue to use libs even if they don´t really need them (yet), just due to general hype around React/JavaScript and its environment. Until this somewhat solidifies and more stable boilerplates/react-frameworks emerge, this won´t change.

Just my 2 cents.

@jlongster
Copy link
Member

@gaearon There is already so much discussion here T_T

I think there are 2 use cases this solves for me:

  1. Makes the redux store represent the entire state of the app (introspect it for debugging, replay, etc). This is not a dev-only feature, in production it's entirely feasible to snapshot the whole state when something goes wrong, or even certain undo-ing features.
  2. Provides the ability to change the location within an action creator. This is very useful because it's not uncommon to have a workflow in an action creator that is something like "authenticate user -> if successful, redirect to /admin".

For everything else I think the raw react-router API should be used. I don't really know if you area solving #2 with your solution, Dan, but I hope we can collaborate on this within this project instead of diverging.

EDIT: The above features are naturally express as the URL in the state, because the user can debug by looking at it and use actions to feel like they are "changing" that URL state even though they are changing history. It's entirely possible that #2 could be solved by passing history into an action creator. The difference between this and fetch is that fetch is a global API already available, which makes it easier to use within action creators.

@jlongster
Copy link
Member

To answer the original comment clearly: 1 was not a goal of this project (for me), but just came out of it, 2 yes, in certain cases providing actions was easier, and was mostly inspired by redux-router. But I don't care much about this, I'll pass history around. Also, react-router singletons make this pain go away completely. 3. Yes this was the #1 goal of this project.

So your approach is fine to me.

@taion
Copy link
Member

taion commented Feb 4, 2016

@jlongster With the v2 API, you generally won't need to pass around history – just use the browserHistory singleton instead.

@jlongster
Copy link
Member

@taion Yeah that's what I meant by "react-router singletons make this pain go away completely". I realized that while typing it out.

EDIT: (and that feature wasn't available when this project started, hence why it works the way it is now. But Dan's new direction is a welcome change in response to RR 2.0)

@taion
Copy link
Member

taion commented Feb 4, 2016

Yeah, I think 95% of users would be ill-served to use action creators v the singletons.

@gaearon
Copy link
Member Author

gaearon commented Feb 4, 2016

I think I have a good enough understanding of the situation now.
Big thanks to everyone in this discussion for bringing very valuable points.

I think having two exports in the future and downplaying the role of the actions one in the README might be a good way to start encouraging people to use history directly. Meanwhile, people relying on those action creators can keep using them until we have a better approach for this.

The focus of this project would then be the history enhancer that routes updates through Redux store, as prototyped in reduxjs/redux#1362. I don’t have time to contribute right now but I’ll try to allocate some time to it in the coming weeks if nobody beats me to it. (Please do beat me to it! 😉 )

@gaearon gaearon closed this as completed Feb 4, 2016
@mjrussell
Copy link

I don't fully understand the dislike directed towards action creators. I get that the singletons are easy to use. But here's a perfect example of where I'd prefer action creators over singletons (even if all it does is called a history singleton to perform the transition).

Consider you are writing a saga in which you want to listen for a specific action say LOGOUT and update the URL. Sure, in this saga you could import the history singleton and just fire a transition, but the point of redux-saga is to return descriptions of what you are doing for easy to understand concurrency and testing. In this way, providing an action creator means I dont have to worry about jsdom, using spy() or anything else in my tests, I can just check that the saga will put (saga term for dispatch) an action creator.

I get its a bit contrived and there are many other ways to handle that scenario, but my point is that so many cool things in redux are built around action creators and there will always be use cases for them. If its simple to add, throw them in and just document that they are not necessary in most cases

@mjrussell
Copy link

Otherwise I really like the direction of all of this. I think it provides a great solution while still being true to the goals of this project and not subjecting users to potential pitfalls

@timdorr
Copy link
Member

timdorr commented Feb 4, 2016

You can certainly come up with use cases for them, but the vast majority of users can avoid them and use just the history singletons. To be clear, action creators will remain in the next version of react-router-redux. They will simply be an optional thing to use and we will deemphasize their use.

@gaearon
Copy link
Member Author

gaearon commented Feb 4, 2016

If its simple to add, throw them in and just document that they are not necessary in most cases

This is exactly what we plan to do. What I’m proposing is decoupling two unrelated parts (“actions as DSL for history method calls” and “enhancing history to keep its state in the store for record/replay”) while keeping them under the same roof. You will be able to use either feature, or both of them.

Don’t forget that many people don’t use sagas, don’t yet write tests for action creators, are just starting, and are completely overwhelmed by how many libraries they need to get something running. So we want to emphasize why you’d want this lib, and what you can do instead if you’d rather not bring it in.

@mjrussell
Copy link

@gaearon yeah I just saw that proposal in your PR, I think thats a great approach. Just saw the trending discussion towards being very anti-action creator and wanted to provide a perspective of a user who is not just using them because its an easier mental model, but actually because they provide a concrete benefit.

@gaearon
Copy link
Member Author

gaearon commented Feb 4, 2016

👍 thanks for chiming in! This discussion informed my perspective both in how action creators are not essential and how they are still quite damn useful to some people in some situations.

@jackgeek
Copy link

jackgeek commented Feb 8, 2016

I'm new to redux and react-router-redux and I'm concerned about the state.routing concern (1) that gaearon raised. Should I avoid reading the location from state? Can someone please expand on when using this would be a bad idea?
Cheers

@timdorr
Copy link
Member

timdorr commented Feb 8, 2016

@jackgeek Nothing inherently wrong with it. The issue comes when you use connect() from react-redux for your route components. React Router is already providing props to you, so listening to the store as well results in double-renders. You can also get yourself into an infinite loop if you dispatch from componentWillReceiveProps. Just some general badness.

If you're accessing state elsewhere, it's not a bad thing.

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

No branches or pull requests