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

React router integration #177

Closed
malte-wessel opened this issue Jun 24, 2015 · 42 comments
Closed

React router integration #177

malte-wessel opened this issue Jun 24, 2015 · 42 comments

Comments

@malte-wessel
Copy link

I'm not sure if this was already considered. I'm currently playing around with a react-router integration and want my actions to return a transition statement:

export function loginSuccess(user) {
  return {
    user: user,
    type: LOGIN_SUCCESS,
    transitionTo: '/dashboard'
  };
}

In my case it's necessary to run the transition after the dispatch is finished. My idea was to compose the middleware stack like this:

const dispatcher = createDispatcher(
    composeStores(reducers),
    (getState, dispatch) => [ thunk(getState), logger, dispatch, transition ]
);

The transition middleware should read the transitionTo key and invoke the routers transition method.
Any idea how this could be done?

@acdlite
Copy link
Collaborator

acdlite commented Jun 24, 2015

This is essentially a side effect. Is there any reason this could not be implemented by subscribing to the store's change event?

@malte-wessel
Copy link
Author

Wouldn't that be a side effect either? How would I get the transitionTo key from the action?
What would you recommend in order to invoke transitions from actions? Is it a good idea at all?
Many questions :)

@acdlite
Copy link
Collaborator

acdlite commented Jun 24, 2015

Make a route reducer that saves the current route state. Then, transition from inside a store subscriber whenever the route state changes.

@malte-wessel
Copy link
Author

makes sense :) thx!

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

Make a route reducer that saves the current route state. Then, transition from inside a store subscriber whenever the route state changes.

I think this is contrary how React Router works. When using RR with Flux, I think it's right to listen to the router's change event and fire app's Actions, which will in turn update the store with the new current path. The root component listens to that store and re-renders.

So the normal flow is [browser event or transitionTo call] --> [router change event] --> [CHANGE_ROUTE app action] --> [reducer] --> [component re-renders]. What you're suggesting seems to be reverse flow: [DO_SOMETHING action] -> [reducer] --> [some root component calls transitionTo]. I don't think these approaches can be combined in a sensible way.

I think that any router transitions should be in action creators—because, effectively, router.transitionTo call turns into an action caused by the route change handler. I'm not sure it makes a lot of sense to abstract this away in the middleware, but without it, I'd write something like this:

export function loginSuccess(user, router) {
  return dispatch => {
    dispatch({ user, type: LOGIN_SUCCESS });
    router.transitionTo('/dashboard'); // will fire CHANGE_ROUTE in its change handler
  };
}

It's up to the calling component to pass the router.

@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

Yeah, that's essentially how I've always done it, but I've never really liked how such a crucial piece of state was out of your control. In an ideal world, we'd keep route state inside our store, right?

It was my understanding that with React Router 1.0, the browser history listening part, the route matching part, and the rendering part are all separated. So it seems like it should be possible to fire an action inside the history listener, and match + render inside the the store listener.

// Fire action creator
// Store keeps track of current location
BrowserHistory.listen(location => updateRouteLocation(location));

store.subscribe(state => {
  // Obviously you would want to memoize this
  Router.match(state.location, (error, props) => {
    // blah blah React.render() goes here
  }
})

Huge disclaimer: I haven't actually implemented this yet, nor have I played with the RR beta, but it seems like it should be possible.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

This is precisely what I'm advocating :-)

My point was that you don't want the custom action (e.g. LOGIN_SUCCESS reducer) to change router store; you want to call router's side effect in the loginSuccess action creator, so that this updates URL bar and causes history to emit change event, which in turn will translate to your CHANGE_ROUTE action and then go as you described.

@malte-wessel
Copy link
Author

What about the router reducer to handle the LOGIN_SUCCESS action like:

export default function router(state = {}, action) {
    switch (action.type) {
        case LOGIN_SUCCESS:
            return {
                location: { pathname: '/' }
            };
        case LOGOUT:
            return {
                location: { pathname: '/login' }
            };
        default:
            return state;
    }
}

Shouldn't the stores/reducers be responsible for changing the state?

@malte-wessel malte-wessel changed the title dispatch as middleware React router integration Jun 25, 2015
@malte-wessel malte-wessel reopened this Jun 25, 2015
@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

@gaearon Re-reading your comment I see we're mostly on the same page. My original comment should have said:

Make a route reducer that saves the current route state. Then, re-render from inside a store subscriber whenever the route state changes.

rather than transition.

I disagree that the action creator is the right place to put router.transitionTo(). That should be in a store subscriber. Yes, that means there are two dispatches, but your example has two dispatches also. The difference is that one approach is reactive and the other is imperative.

EDIT: To elaborate

store.subscribe(state => {
  if (state.loggedIn && state.location == something) {
    router.transitionTo(successLocation);
  }
});

or something like that, you get the idea.

@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

@malte-wessel A core part of the Redux architecture is that we assume reducers are pure, with no side-effects. It's what enables stuff cool stuff time travel.

@malte-wessel
Copy link
Author

I have now something like this

function handleHistoryChange() {
    redux.dispatch(historyChange({ history.location }));
}

history.addChangeListener(handleHistoryChange);

function handleRouteStateChange() {
    const location = redux.getState().router.location;
    // distinguish between changes that come from history 
    // (like pressing the back button) and those from actions... 
    // If the change comes from history return here (history is already in this state)
   const fromHistory = redux.getState().router.fromHistory
   if(fromHistory) return;

    const { pathname, query, state } = location;
    // You need to implement your own transitionTo method
    history.pushState(state, makePath(pathname, query));
}

redux.subscribe(handleRouteStateChange);

The history is shared with the router, so you don't have to implement your own router rendering
In the route reducer you would do:

export default function router(state = {}, action) {
    switch (action.type) {
        case HISTORY_CHANGE:
            return {
                location: action.location,
                fromHistory: true
            };
        case LOGIN_SUCCESS:
            return {
                location: { pathname: '/' }
            };
        case LOGOUT:
            return {
                location: { pathname: '/login' }
            };
        default:
            return state;
    }
}

@acdlite Isn't the route reducer pure? I think you could actually do time travel with this.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

@acdlite

I still think this goes against how RR works. RR can cancel the transition, for example. (Route onEnter hook could redirect.)

In your example, the app would first think it's on the success page (and re-render), but then the router's transition method would be called, and router might redirect the user from a route that's not ready yet.

For routing state, I think the router change event payload should be the source of truth. Otherwise the router is not in control. Our wish to transition is our wish, but ultimately the router should decide when and whether it happens.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

Here's another example. RR 1.0 supports async route configuration. This means that you might want to transition to /something, but this route is not even loaded yet!

If the router is in control, you tell it to transitionTo and whenever it's ready for this to be the active route, it will tell us that via a change handler. But if the router is not in control, we'll have nothing to render. There's just no handler for /something yet.

@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

@gaearon How is RR not in control? All I did was change the location of router.transitionTo() from the action creator to the store subscriber. state.location is always up to date because of the BrowserHistory listener. Clearly I'm missing something...

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

@acdlite

With this approach:

export default function router(state = {}, action) {
    switch (action.type) {
        case HISTORY_CHANGE:
            return {
                location: action.location
            };
        case LOGIN_SUCCESS: // <------------ I don't think it's a good idea!
            return {
                location: { pathname: '/' }
            };

Location is updated in response to user action (LOGIN_SUCCESS) even if the corresponding route configuration might not have loaded yet.

All I'm saying is, the only thing that can change state.location is the route reducer which reacts only to the router change action. It does not react to any user action.

With your code sample:

store.subscribe(state => {
  if (state.loggedIn && state.location == something) {
    router.transitionTo(successLocation);
  }
});

I assume that state.location was somehow changed in response to the user action. Is something meant to refer to the previous position? Then I misunderstood you.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

I also don't think reactive approach makes sense here.

This:

store.subscribe(state => {
  if (state.loggedIn && state.location == something) {
    router.transitionTo(successLocation);
  }
});

will also trigger if I press “Back”. The state.location resets to the location where I was, and loggedIn is true, which will redirect me forward again.

My point is, it's clearly a side effect, and it's a side effect relevant to a particular action creator. It's not a global side effect that happens every time you're on a specific page and a specific field is true.

Therefore I think it belongs in the action creator.

@malte-wessel
Copy link
Author

In RR 1.0 you don't have access to the router instance until it's rendered anywhere. This makes things even more complicated. I guess that if you do a history.pushState() the router will try to make the transition. If the transition is aborted the router will update the history again (I guess) and the router store/reducer will be in the correct state.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

From the components, you always have access to the router instance. (It's passed by context.)

@malte-wessel
Copy link
Author

yes, but not from the action creators. except when you store a reference to the router when it's rendered. but I think thats not the way to go.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

yes, but not from the action creators.

You can pass the router instance to the action creators. You can also transition from the components themselves.

@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

I assume that state.location was somehow changed in response to the user action

Nah, that is only changed by updateRouteLocation() in my comment above, so it's always in sync:

BrowserHistory.listen(location => updateRouteLocation(location));

It's not a global side effect that happens every time you're on a specific page and a specific field is true.

What does a "global" side effect mean in the context of Redux, where the entire state atom is emitted every single time? Yes, my example above is clearly naive. In reality you would only be responding to a subset of the state changes, either using Connector's select, using observables, etc.

It's just like using componentWillReceiveProps() to trigger a side effect.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

In reality you would only be responding to a subset of the state changes, either using Connector's select, using observables, etc.

I just don't understand how you can tell between situation where you need to redirect (just authenticated) and where you don't need to redirect (authenticated long ago but state looks similar).

It sounds much more complicated than just doing it imperatively wherever you're dispatching LOGIN_SUCCESS.

@gaearon
Copy link
Contributor

gaearon commented Jun 25, 2015

(In fact I tried both approaches when developing Stampsy. I had too much pain with “observable” approach when some new conditions caused some filter I wrote long ago to fire and cause unwanted redirects. In the end I went with the imperative approach and haven't had a problem ever since.)

@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

@gaearon Example:

const stateStream = storeToStateStream(store);

stateStream
  .distinctUntilChanged(state => !state.loggedIn && state.location === '/login')
  .subscribe(state => {
    if (state.loggedIn && state.location === '/login') {
      // Only fires when you're on the login page and you just logged in
      router.transitionTo('/success');
    }
  });

Edit: even better...

const stateStream = storeToStateStream(store);

stateStream
  .distinctUntilChanged(state => !state.loggedIn && state.location === '/login')
  .filter(state => state.loggedIn && state.location === '/login')
  .subscribe({
    router.transitionTo('/success');
  });

@acdlite
Copy link
Collaborator

acdlite commented Jun 25, 2015

I like this reactive approach better because now it doesn't matter how the user logs in as long as the state field has changed. You can use a login form, a web socket event, whatever... no need for separate transitionTo() calls.

Clearly not all transitions should be done this way (not even most), but I think it's best for transitions that are side effects of Flux actions.

@gaearon
Copy link
Contributor

gaearon commented Jun 26, 2015

Oh, I actually like it. I missed the distinct and “changed” bit.

@istarkov
Copy link
Contributor

Also moving router.transitionTo from action creators to store subscriber makes code more independent of router realization, no need to pass router instance to action creators. (i can use middleware to remove this dep)

But even for simple changeRoute action I need something like this

const stateStream = storeToStateStream(store);

stateStream
  .distinctUntilChanged(state => state.requestedRoutePath)
  .filter(state => !!state.requestedRoutePath)
  .filter(state => state.requestedRoutePath !== state.location)
  .subscribe({
     router.transitionTo(state.requestedRoutePath);
   });

(It will not work if location changed elsewhere but requestedRoutePath still the same so need reset it in reducer on location change.)

Moving 'router.transitionTo' to action, makes code simple and life more easier.

@acdlite
Copy link
Collaborator

acdlite commented Jun 26, 2015

@istarkov I wouldn't use a generic changeRoute action except for in the BrowserHistory listener. Just use router.transitionTo() directly. React Router will handle comparing the current route the previous route, comparing to the current location, and all that stuff.

My point about doing transitions inside state subscribers regards only those route transitions that are a side effect of an action — like when a user logs in, or when a form is successfully submitted, etc.

Good catch on using distinctUntilChanged() instead of distinct(), though. I updated my example above.

@timothyjlaurent
Copy link

Is there a library or code sample used for the storeToStateStream function?? I'd love to see a more fleshed out version of the BrowserHistory Listener.

@tappleby
Copy link
Contributor

tappleby commented Jul 1, 2015

@timothyjlaurent the storeToStateStream function returns a RxJs Observable:

function storeToStateStream(store) {
  return Rx.Observable.fromEventPattern(
    (handler) => store.subscribe(handler),
    (handler, unsubscribe) => unsubscribe(),
    () => store.getState()
  );
}

@timothyjlaurent
Copy link

@tappleby thanks for kindly clarifying

@iest
Copy link

iest commented Jul 2, 2015

Still having trouble figuring out using redux with RR...

Specifically, RR 1.0 allows you to pass in an onEnter function when defining your routes like so:

function requireAuth(nextState, transition) {
  // My user state is in redux — how do I get it out!
}

export default (
  <Route path="/" component={App}>
    <Route path="login" component={LoginRoute}/>
    <Route path="account" component={AccountRoute} onEnter={requireAuth}/>
    <Route path="*" component={NotFound}/>
  </Route>
);

I've tried (and failed) at having a redux singleton contained within a module.

The problem is I need the same instance of redux both inside my route definition (for the auth handler) and inside the client entry point (where I hydrate the redux instance and pass it into the Provider). I'm doing server-side rendering for added fun!

Any ideas? Can I use context here somehow?

@Dr-Nikson
Copy link

@iest you can wrap your routes definition:

// routes.js
export const createRoutes = (requireAccess) => {
  return (
    <Route name="app" component={App}>
      <Route name="home" path="/" components={HomePage} onEnter={requireAccess(accessLevels.user)}/>
      ...
    </Route>
  );
};

And then bind requireAuth to redux and pass it to createRoutes.
I'm working on auth example, so you can check it out https://gist.github.com/iNikNik/1eabfc6bcc132384368c (It's VERY basic version, but I improve it)

@guillaume86
Copy link

@gaearon Is there a gist somewhere with the current "best" practice (I know it's moving fast at the moment) to integrate RR1.0 with redux?

@iest
Copy link

iest commented Jul 5, 2015

Looks perfect @iNikNik! Thanks very much.

@gaearon
Copy link
Contributor

gaearon commented Jul 20, 2015

While it's far from perfect API right now, something good (and official) is going to eventually come out of https://github.com/acdlite/redux-react-router so you might wanna watch it.

Leaving this open so I'll come back to this in new docs.

@guotie
Copy link

guotie commented Aug 30, 2015

maybe react-router not suit for redux?

@rightaway
Copy link

+1

@gaearon
Copy link
Contributor

gaearon commented Sep 24, 2015

Superseded by #637 which contains more up-to-date info.

@aaeronn
Copy link

aaeronn commented Jan 28, 2016

@gaearon I am following approach for routing in action.. I am new to react and redux and got following issue:

  1. router is not defined in router.transitionTo("/url")
  2. I have set up url correctly and its working to.. Lets say I have homepage with tow tabs one and two which redirects to /one and /two .. when I click from homepage on that tabs it properly redirects but when I do something like localhost:3333/one from browser it gives me error.. url is not defined

Any suggestion ?

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

Please ask this on StackOverflow as this is a usage question. We have real-world example in the repo that implements routing.

@aaeronn
Copy link

aaeronn commented Jan 28, 2016

I have asked it but no one answered :) ... http://stackoverflow.com/questions/35057443/redux-transition-to-after-action-execution

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