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

Support for React-Router 4 #107

Closed
sebmor opened this issue Feb 9, 2017 · 33 comments
Closed

Support for React-Router 4 #107

sebmor opened this issue Feb 9, 2017 · 33 comments

Comments

@sebmor
Copy link

sebmor commented Feb 9, 2017

Since it's somewhat officially released now, any plans for redux-connect to support it?

https://reacttraining.com/react-router/

@AVVS
Copy link
Member

AVVS commented Feb 9, 2017

Well, I havent personally invested time in understanding the differences between v4 and v3, but so far it looks like routes would instead be simple components, hence there might be a need to revisit the whole thing on how we evaluate render tree and gather async props

@Rmannn
Copy link

Rmannn commented Feb 22, 2017

I wrote a module that works with react router v4, but it's experimental.
https://github.com/Pop-Code/redux-async-load
Maybe this could help.

@AVVS
Copy link
Member

AVVS commented Feb 24, 2017

@Rmannn thanks, though this requires us to render something 2 times. Its a huge blocking overhead I'd like to avoid at all costs possible.

@AVVS
Copy link
Member

AVVS commented Mar 14, 2017

screen shot 2017-03-14 at 2 10 58 pm

Once the bottom 2 repos are stable I assume we can actually integrate this module with the new react-router, since they can still match the routes before hand and if we can still do that - then we can still prefetch data

@harmonyjs
Copy link

Any updates?
These packages are quite stable and already ready for use, IMO

@AVVS
Copy link
Member

AVVS commented Apr 27, 2017

I think I'd have time to investigate router version 4 futher soon, but if any1 wants to budge in with some help before that happens - let me know

@oyeanuj
Copy link

oyeanuj commented May 27, 2017

@AVVS Just checking to see if you've had a chance to look at RR4 to update this package?

@AVVS
Copy link
Member

AVVS commented May 28, 2017

I did, but it still doesnt support redirects and some of the other configuration features. It's definitely possible to integrate as there are examples & docs, but the solution would be incomplete, so I'm quite hesitant at this point to invest time in this as I personally can't use it in production on any of the projects I'm involved with, I'm revisiting this every few weeks though to see if there is any progress on that front

@finom
Copy link

finom commented Jun 26, 2017

@AVVS any news?

@AVVS
Copy link
Member

AVVS commented Jun 26, 2017

no progress on their front for the modules in question, so no news here either

@andykais
Copy link

andykais commented Jul 10, 2017

according to this stackoverflow post the way to accomplish redirects is through the history object within Router of 'react-router-dom'. Aka, createBrowserHistory(), then pass that instance to react-router AND redux-connect and then you can programatically do most routing things using the
const { push, replace, go, goBack, goForward } = history api

Its actually even simpler to get the history instance shown in this official example, you can pull history out of react-router-dom with withRouter function

does this satisfy all the requirements to start supporting v4?

@jchook
Copy link

jchook commented Jul 13, 2017

Since this package is slow to adopt React Router 4.x, we just factored it out of our app.

It really wasn't too hard. Router 4.x plus redux-saga makes it trivial to preload data for your app in a uniform way at any level of the component tree. We just wrote a decorator to automatically inject a loading animation when this.props.loading === true.

@AVVS
Copy link
Member

AVVS commented Jul 13, 2017

thats another way to do it, I'd suggest you make a README entry here for those who want react-router v4 support. For me it really drops down to hacks that are needed to make this work (even though it can certainly happen)

@jchook please do provide your solution here via a PR - it would be superb to give more options to people!

@jaraquistain
Copy link

@jchook does your implementation work on the server as well? Will it wait until all external async data is returned before sending a response?

@jchook
Copy link

jchook commented Jul 17, 2017

@jaraquistain unfortunately it does not. We didn't need "universal" rendering support so we removed it during the refactor.

I can work on a README in my spare time for folks that don't need server-side rendering. Otherwise, I think @andykais is correct about withRouter, etc and that 4.x support should be feasible.

@AVVS
Copy link
Member

AVVS commented Jul 17, 2017 via email

@jaraquistain
Copy link

@AVVS That's precisely what @jchook is saying, correct? He removed this module entirely in favor of loading screens and browser rendering.

Either way, I'll keep checking here for progress as it seems like if the statements above are true then something should be feasible soon

@jchook
Copy link

jchook commented Jul 17, 2017

Sorry for any confusion about that.

I'm trying to theorize a way to accomplish SSR in this way. Since 4.x allows routes nested deep within the component tree, the technique/implementation may need to change significantly, and I don't imagine it will be backwards compatible.

Here is an ignorant and likely oversimplified description of how I imagine it working:

  1. decorator provides most of the functionality
    a. stores async promises in an array
    b. loading and error indicators/callbacks (for client-side rendering)
    c. connecting state to props as configured (when promise is resolved)
  2. server.js awaits promises stored by initial render
  3. then respond with final, pre-loaded HTML

@jchook
Copy link

jchook commented Jul 18, 2017

Also, it looks like react-router is developing a package called react-router-config for 4.x that will (hopefully?) provide an official solution.

@AVVS
Copy link
Member

AVVS commented Jul 18, 2017 via email

@adam-26
Copy link

adam-26 commented Aug 27, 2017

I forked this project and modified it to support react-router v4, the API is completely different - so I'm not sure if it will be of interest, but given the changes in rr-v4 I don't think a migration could maintain the current API of this project.

Given the API change, i've renamed it react-router-dispatcher, its available on NPM. I'd be interested in any feedback.

@AVVS
Copy link
Member

AVVS commented Aug 28, 2017

It would be great if you prepare a PR and we can review it & publish under new branch version and gradually switch over there, since there is a lot of demand for RR4!

@oyeanuj
Copy link

oyeanuj commented Aug 31, 2017

@AVVS Instead of waiting on react-router-config or making that a requirement, could we use something like React Tree-Walker instead to compute the data dependencies?

Unless of course, you think merging @adam-26 package is a better strategy.

@oyeanuj
Copy link

oyeanuj commented Aug 31, 2017

@AVVS Couple of more things -

  1. It seems like a few libraries are now using react-router-config - do you think it makes sense to have it a next branch to have a solution and you can make it the stable branch once react-router-config goes out of beta?

  2. Here is another strategy that I ran into someone using for async components with RR4, but could totally be useful for redux-connect: Upgrade to React@16 (next) ctrlplusb/react-universally#479 (comment)

@adam-26
Copy link

adam-26 commented Sep 1, 2017

@oyeanuj, just so you are aware, the async components you mention above performs more than 1 render per-request when rendering server-side, it has to make a first pass of the render tree to determine the components to be rendered, then another pass to actually render the components. The first render is not a complete render. I don't know the exact performance hit, but probably not ideal if you're site receives a lot of traffic.

@oyeanuj
Copy link

oyeanuj commented Sep 1, 2017

@adam-26 Yes, totally. Infact, a little further below in that thread, the author of the comment does address that - ctrlplusb/react-universally#479 (comment). TLDR: His opinion is that as a discovery algorithm, it is not nearly as much as the render cost when they measured.

It seems like the alternatives would be to either just declare data dependencies on the top container or at a route level - in both of which cases, we lose a lot of flexibility. Like everything else, tradeoffs 😄

@sebmor
Copy link
Author

sebmor commented Sep 13, 2017

Check also this package https://github.com/gabrielbull/react-router-server

Also this example repo of another implementation is also pretty cool, he used getInitialState similar to NextJS https://github.com/jaredpalmer/react-router-nextjs-like-data-fetching

@oyeanuj
Copy link

oyeanuj commented Sep 25, 2017

@AVVS Any thoughts based on this comment and below? Seems like there are a few different ways to explore - I'm curious to see which one feels more like the right path for you?

@AVVS
Copy link
Member

AVVS commented Sep 25, 2017

react-router-config is definitely a must, then match the tree and walk using it. My main concern was handling redirects, and some other features I've tried tackling were not supported as well.

I'd be grateful if some1 can at least provide a set of tests to iterate upon and then I'd be able to adjust the code

@warrenc5
Copy link

warrenc5 commented Nov 22, 2017

Thought I'd share some of my experience here,

At first I thought I was insane to try to upgrade this module, but then I read the documentation about react and redux and used the chrome debugger to construct a working example of the async using v4 routing.

I'm sure it's super sub-optimal and lame but it did allowed me to port my app to r16.

Basically, I had to remove the RouterContext import/require from the components/AsyncConnect source code.

I also had to remove component from the props to route, because you can't have component and override render for ReduxAsyncConnect constructor or otherwise react will ignore render.

Using the debugger I could see how the connect would put a static reduxAsyncConnect map of keyed promises on the connected component.

And then when the <ReduxAsyncConnect> mounts it resolves all the static map promises and dispatches the promises thened result to redux on their key. ( Or something to that effect)

const Rac = ({component,...props})=>
    <ReduxAsyncConnect {... props}
        components={[component]} reloadOnPropsChange={reloadOnPropsChange}
        render={props=>React.createElement(component,props)}
    />

const AsyncRoute = ({component, ...props}) => (
    <Route {...props} render={props => <Rac component={component} {... props} />}/>
)

I further extended the routing to a static component route ... although my Login component is also MyComponent and rather than use a redux route dispatcher or a redirect in the componentDidCatch method I decided to do it with a v4 Redirect as the following shows.


export class RouteCatch extends MyComponent {
    render() {
        const {component:Component, ... rest} = this.props
        const isError = super.isError()
        return <Route {... rest} render={props => (
            isError?
               <Redirect to={{
                    pathname: ERROR,
                  }}/>
            : <Component {...props} />
            )}/>
    }
}

const Rac = ({component,...props})=>
    <ReduxAsyncConnect {... props}
        components={[component]} reloadOnPropsChange={reloadOnPropsChange}
        render={props=>(<RouteCatch component={component}/>)}
    />

const AsyncRoute = ({component, ...props}) => (
    <Route {...props} render={props => <Rac component={component} {... props} />}/>
)

And my use is

 <Route path={ERROR} component={Logout} pageTitle="Error"/>
                        <AsyncRoute exact path={LOGIN} component={Login} pageTitle="Sign In"/>
                        

Thanks for all your work and support with this module. It's clearly the easiest to use for the big win without heavy weight middleware.

@AVVS
Copy link
Member

AVVS commented Dec 9, 2017

As promised quite a long time ago - #119, its WIP, but will handle most of the cases

@AVVS
Copy link
Member

AVVS commented Jan 5, 2018

Closed in https://github.com/makeomatic/redux-connect/releases/tag/v7.0.0

@AVVS AVVS closed this as completed Jan 5, 2018
@bertho-zero
Copy link

By switching from react-router 3 to 4, I dropped redux-connect to create my own component (https://github.com/bertho-zero/react-redux-universal-hot-example/blob/master/src/components/ReduxAsyncConnect/ReduxAsyncConnect.js), I'd like to go back to redux-connect now.

I find myself with something very similar but I added Nprogress to show the page loadings as on github or youtube, and I don't know how I could integrate that with redux-connect, maybe you could help me to return to redux-connect.

At home (L32 + L58):
https://github.com/bertho-zero/react-redux-universal-hot-example/blob/04489fb51dd994d182bad540fd92ef45b0e20671/src/components/ReduxAsyncConnect/ReduxAsyncConnect.js#L24-L60

Redux-connect:

loadAsyncData(props) {
const { store } = this.context;
const loadResult = loadAsyncConnect({ ...props, store });
this.setState({ previousLocation: this.props.location });
// TODO: think of a better solution to a problem?
this.loadDataCounter += 1;
this.props.beginGlobalLoad();
return (loadDataCounterOriginal => loadResult.then(() => {
// We need to change propsToShow only if loadAsyncData that called this promise
// is the last invocation of loadAsyncData method. Otherwise we can face a situation
// when user is changing route several times and we finally show him route that has
// loaded props last time and not the last called route
if (this.loadDataCounter === loadDataCounterOriginal && this.mounted !== false) {
this.setState({ previousLocation: null });
}
// TODO: investigate race conditions
// do we need to call this if it's not last invocation?
this.props.endGlobalLoad();
}))(this.loadDataCounter);
}

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

No branches or pull requests