`onAfterChange` and redirects

[sdougbrown]

Before diving into a PR, I'd like to discuss this to determine whether the behaviour is intended.

Currently, onAfterChange runs even when the route redirects. This can result in onAfterChange running twice with the same result, as the middleware will have already changed the current page (via getState()).

Some of this could be due to a race condition in my particular case (async functions), but it seems to be that the change should "resolve" prior to running a function like onAfterChange.

Agree or disagree?

[faceyspacey]

In a regular use case it absolutely should not be called, and if it does, it's a bug. check these lines:

https://github.com/faceyspacey/redux-first-router/blob/master/src/connectRoutes.js#L281

https://github.com/faceyspacey/redux-first-router/blob/master/src/connectRoutes.js#L302

https://github.com/faceyspacey/redux-first-router/blob/master/src/connectRoutes.js#L254

If you're using async functions in onBeforeChange, it's because they are not yet supported there. Here's the discussion about how to implement it:

https://github.com/faceyspacey/redux-first-router/issues/90#issuecomment-324740892

It actually shouldn't be that difficult. A PR would definitely be appreciated.

[sdougbrown]

As it turns out, it was because in my case I was declaring redirects in the route thunks, rather than a global onBeforeChange. #101 seems to fix it.

[faceyspacey]

oh i c. yea, the system wasn't designed for synchronously called route thunks to have this effect. i like how you handled it.

i still gotta think a little bit more about it. what do you think about the fact that if the redirect is dispatched asynchronously (e.g. after getting some data) it won't be applied? It's a big inconsistent, but I think it's likely fine--i mean there isn't anything else we can do.

Now that said, I'm not 100% sure we do want to block this onAfterChange. If you check the top of the repo you'll see in the @next tag on npm there are some new features, one being a 3rd argument to thunks and callbacks containing a "bag." I.e. an object. We could put some info in there about how the user arrived at the given callback, and leave it up to the user. The thing is this: if the thunk is being called, the route did in fact change already.

If the thunk we called before the route changed, then this would make 100% sense. It makes me want to dispatch the thunk around the time onBeforeChange is called instead. The reason I don't is because thunks need the updated reducer state. I mean we could do some crazy stuff we're clone the store and reduce the action to get the result so that the getState arg passed to the thunk reflects what the store would be. But that sounds problematic etc.

Anyway, imagine this:

onAfterChange: (dispatch, getState, { action, extra, info }) => {
   if (info === 'isRedirect') // do something different
}

Now that said, you already have the info that it's a redirect here:

getState().location.kind === 'redirect'

So essentially you can already accomplish what you want to do (and have further facilitated) in userland. Just check if it's a redirect.

Now, I don't want to make the decision lazily so as not to have to do the work of integrating this. I was about to integrate it, and apply it to the latest code (the code has changed a lot lately). It's just problematic since the route did in fact change.

Here's another thing: basically you shouldn't redirect synchronously in a thunk. Only do it later asynchronously after some actions happen. That's what onBeforeChange is for. That's the only way to effectively redirect before lots of other things happen. A redirect happening immediately would lead to other things happening incorrectly.

Now the challenge is then u gotta detect types in onBeforeChange. That's all it boils down to. That would solve your problem. It's a bit less than ideal. But it's better than adding onBeforeChange hooks to each route, as we want to keep the API surface as small as possible or the package becomes essentially worse. Less is more.

One thing I've considered is making a 3rd party package (or perhaps u want to) that you assign to onAfter/BeforeChange like this:

import { routeLevelOnBeforeChangePackage } from 'rfr-helpers'

options = {
    onBeforeChange: routeLevelOnBeforeChangePackage({
         TYPE: (dispatch, getState) => ...
         ETC: ...
    }, (dispatch, getState) => {
       // catch all
    }),
    onAfterChange: // ...same stuff as onBeforeChange
}

I can link it from the docs, etc. But the point is not to introduce a million features when in javascript doing the above is ultimately easy. U dont need a package to do that nicely. Just make that function and you get access to types there again. OR!, here's a better way:

const routesMap = {
   TYPE: {
        // the usual suspects,
        onBeforeChange: ...
    }
}

const options = {
    onBeforeChange: yourPackage(routesMap, (dispatch, getState) => ...)
}

What is that doing? It's doing the same as the first one, but using an arbitrary key on routes, just the way you would want it.

Anyway, there are more challenges than just blocking onAfterChange. I'm worried about many other things functioning correctly. Both in the package code and userland. I mean the URL changes for a quick second here, whereas with onBeforeChange it doesn't. That's why it's absolutely better to do this all in onBeforeChange. There's many things that happen, from scroll restoration, to pushing vs replacing the URL, to confirmLeave modals (which is new and only on NPM in @next), page title changing, and still more. Why would you want to do this the jerry-rigged way when onBeforeChange is not only the current way to do it, but really the only way to do it fully correctly. You have complete capability to prevent things from happening--u just gotta do it in onBeforeChange. Things have to be this way--cuz thunks have to run after the state has updated. That's the primary thing. There's no way to avoid that. So no matter what--an "after state" has been reached, even if just for a little bit of time. Those additional renders with redirects will make jank happen if you have a highly animated app. I try to make no unnecessary renders happen ever. That's always my goal. Many apps never witness the jank that's possible with React--cuz they don't build apps that try to mimic what an Apple/Fire TV app looks like. Once you do in the browser, you realize React has horrible performance. And Fiber async won't solve it. So that means even more reasons to do things in onBeforeChange where state won't change if u do a redirect.

I think the above yourPackage strategy is an excellent strategy. I'll link up the repo in this repo, etc. There's just need for this repo to concern itself. I mean I'm tempted, but it saves so little code, it's just not worth making the library seem more complicated to newcomers looking at it (as well as experienced RFR users). It's just one more thing to think about. I.e. onBeforeChange existing on routes. But if someone else made a package, that's awesome. After all, the modular style of coding React/NPM has made so popular and powerful is all about skipping the automagic of Rails and gaining deep control through realizing your own power to write these little functions.

So I think the best solution is just write some custom code in options.onBeforeChange that will analyze route types just the way RFR does internally. And if a package did that, well, then you don't really have the problem ur trying to solve. Now just define an onBeforeChange on your route which redirects, and then re-use your routesmap in onBeforeChange to make this determination. With or without package helper :)

I look forward to your thoughts. If you're adamant and it's truly the right approach, and something is wrong with what im saying, well then we gotta do the way your PR does. Let me know.

[faceyspacey]

You know, i've been thinking about it more, and one thing I'm thinking is I don't like how some package maintainers are so strict, and ultimately I think what you want would makes for the greatest good. Cuz people will run into it a lot, and we could be as strict as the Redux people for example and just say "ur doing it wrong; u have to do it this way" as for example as they do with not doing dispatching in lifecycle handlers like componentDidMount. But simply that still leaves a wide hole open in the system. So many developers do dispatching in will or did mount only to realize one doesn't work in SSR, and the other one doesn't in fact receive the new state as props! (only grand children do). It adds annoying complexity but smoothing these edges leads to important developer happiness in cases that are all too common.

Also, we may have to introduce the change callbacks at the route level too. I gotta think a little bit more before committing to such major API changes. But rest assured we'll solve your use case, and hopefully a lot of others. And it will be this week.

[sdougbrown]

Yeah all I'm looking to accomplish here is have route-defined redirects. Currently accomplishing that with thunks, that need to run after change (as you outlined above). That's the only reason my PR looked at skipping in onAfterChange. If each route could utilize onBeforeChange instead of a thunk param to synchronously redirect then that'd solve things for my case.

[sdougbrown]

I'm going to test out a custom onBeforeChange handler that utilizes the routeMap internally -- as you suggested -- and release it when I'm satisfied with how it works. Will provide a link here. Thanks for the suggestion. 😄

[sdougbrown]

Actually it's pretty simple as a userland thing...

function createBeforeChangeHandler(routesMap: {
  [key: string]: Object | string,
}) {
  return function onBeforeChange(dispatch: *, getState: *, action: Action) {
    const route = routesMap[action.type];
    if (
      typeof route !== 'object' ||
      typeof route.onBeforeChange !== 'function'
    ) {
      return;
    }
    route.onBeforeChange(dispatch, getState, action);
  };
}

Up to you if you want RFR to be able to do this out-of-the-box. It seems like route-defined redirects should be easier (not that this is hard now that I have it all together).

[faceyspacey]

Been caught up with some client work, but I've decided I'm gonna go the mile on this one and all the related before change promises, redirects and route level versions of it. It's just needed after all.

[faceyspacey]

this is done. Additional info is in a comment to the PR:

https://github.com/faceyspacey/redux-first-router/pull/101