-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Include redux-router v1.0.0-beta3 #261
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
Conversation
0916566
to
d2a493f
Compare
I'm disappointed with the behavior of pushState occurring before the transition is complete. There are legitimate use cases for asynchronously preventing navigation to a route. It's still probably worthwhile to move to redux-router now and try and figure out a solution whenever we can. |
I think any This solution is IMHO ideal for simple react-router implementation, even if you have redux but not once you add redux-router. To me the the middleware component makes it very easy to reason about your app state, is unlikely to break the router (because it can do whatever it wants) and is unlikely to break the app (because again, it can do whatever it wants). Any solution that ties deeper into react-router (such as using history) is likely to need more maintenance in the long-term. I think when you add redux-router into the mix, this might make more sense as middleware (and a new reducer) that captures the new router state, creates a new action along the lines of WILL_TRANSITION with the information and returns a promise that then nexts the original I think that would even work on the server with some wrangling... ? I don't know. Going to try something like that later. |
@apapirovski Yes, that's a limitation of this approach. Great idea, might have a play around tomorrow. |
d2a493f
to
59d6b52
Compare
@apapirovski I've updated to an initial middleware based approach |
59d6b52
to
2858d4d
Compare
@stevoland There's an important consideration with this implementation (that I haven't found a work-around for). You cannot use Given that everything is already coming down in 'props', I don't think this should be an issue if we're truly serious about keeping our state as far up the app as possible. That said, I think it's important for it to be clearly documented somewhere. Also, for what it's worth, it's possible to make this work on the server, in an almost universal way. You just need a store to keep the promise in and then subscribe to that in your server.js, and only render when it's complete. |
3698669
to
193192d
Compare
Hey @stevoland, don't know if the info is relevant, but check out #292 -- some things Anything particular holding this up? Should we wait for redux-router to hit RC first? |
Not sure what's holding this up really. Just a general lack of interest maybe? The issues that I have found are down to redux-router such as keeping React components in Redux state. Development seems to have slowed on that - I get the impression he's waiting for a stable react-router release first. Maybe we should just keep this lying around. Would be good to get more people testing it out though. |
193192d
to
0a4f19b
Compare
Well, the Travis build is failing, for one. Something about |
Odd, just happened after I rebased and only with npm 2. |
`history` module can't resolve `qs` and `deep-equal` so include them in the project
We should probably force npm@3 on Travis. We are on the bleeding edge after all 😄 |
Include redux-router v1.0.0-beta3
Edit: Update to initial middleware approach
Edit: Now fixes
RequireLogin.onEnter
hook. #242#41
In this implementation we're back to the URL updating to the new route before the data fetching transition finishes. I kind of like this. It would also be easy to add a global spinner or something.
You can also now add
status={404}
or whatever to a route change the status code sent with the server render.I'm sure something will be broken!
Breaking changes I can think of:
fetchData
s signature is nowfetchData(getState, dispatch, location, params)
Known issues