Infinite loop when dispatching redirect from within a componentWillReceiveProps

[mattkrick]

if I dispatch a push or replace from within a componentWillReceiveProps, it causes that component to receive new props, making for an infinite loop (at least I think that's the process, chrome profiler stops after the first loop, so I haven't traced it entirely). Would it be possible to do something like save the location.key and if that key comes up twice in a row to just ignore the dispatch? Oddly enough, it doesn't happen if the redirect is to the /...

[timdorr]

Why are you dispatching anything within a cWRP? That seems like a bad code pattern.

[mattkrick]

Because onEnter doesn't work async, so I have to use a HOC to check for auth. If you know of a better way to automatically log someone in I'm all ears!
http://stackoverflow.com/questions/34620435/react-router-auto-login-in-the-onenter-hook
https://github.com/mattkrick/meatier/blob/master/src/universal/decorators/requireAuth/requireAuth.js#L19

[timdorr]

It's kind of ugly, but you can include a simple circuit breaker in there.

let checkingAuth = false;
//....
    checkForAuth(props) {
      if (checkingAuth) return;
      checkingAuth = true;
      const {dispatch, hasAuthError, location} = props;
      let newKey = location && location.key || 'none';
      const authToken = localStorage.getItem(socketOptions.authTokenName);
      if (hasAuthError || !authToken && newKey !== key) {
        key = newKey;
        dispatch(push('/login?next=%2Fkanban'));
      } else {
        checkingAuth = false;
      }
    }

[mattkrick]

Ah, that code already has a circuit breaker in place (newKey !== key). It's doing exactly what this issue proposes, but it's done external to redux-simple-router. I figured a package that's built for react should probably place nicely with react & that it'd be useful to move this logic inside the package. Otherwise, it should definitely be called out that you CANNOT use this package within cWRP & that the result will be an infinite loop that makes you manually kill the tab... but that kinda kills the developer experience.

[timdorr]

The loop is a common one: push -> connect -> React lifecycle -> push, etc etc. We had talked about it in remix-run/react-router#2919, but I'm not sure what the fix should ultimately be. Namely because it crosses so many module boundaries, I'm not even sure who should be responsible for fixing it 😖

[mattkrick]

I definitely understand (I think it's the same bug that bit me in an onEnter hook a couple days ago). That issue helped me make sense of what I was seeing in the debugger, thanks! I know my solution is just a fix for the symptom.
You've got a much better understanding of how all the rackt products fit together so I probably can't be of much use, but from an outside perspective, it'd seem to make sense to have a self-destructing listener (like a listenOnce) but that'd require some changes across the board. I could see it being useful an an external redux API too, but that might kill the dead-simple nature of redux.

[mjrussell]

I agree with @mattkrick that the 'community best practices' right now for auth + redux + react-router seems to be a HOC with dispatches in cWM and cWRP. Its a common answer on discord and there are several examples out there.

I have a similar Authentication/Authorization wrapper in my own app using this approach, but have not had the infinite loop issues at all described here. I am, however, using connect which may protect me against some double renders because of pure defaulting to true.

I think it may be necessary to either come up with a best practice solution for this that is explicitly stated in the README and warns against potential places for error, or to actually add a rackt approved auth module to the ecosystem which everyone can use that handles these potential edge cases when using redux + routing.

[timdorr]

...add a rackt approved auth module to the ecosystem which everyone can use that handles these potential edge cases when using redux + routing.

That wouldn't be a terrible thing to build. I know @ryanflorence and @mjackson want to keep growing Rackt, so if anyone in the community wants to step up and build something awesome for this, that would be a great entry point (webpack pun intended) into the org.

[mjrussell]

if anyone in the community wants to step up and build something awesome for this, that would be a great entry point (webpack pun intended) into the org.

Challenge accepted!

I've got some scaffolding Im pretty happy with that should be pretty flexible for most use cases and tests/examples as well. I'll work on getting it pushed and hopefully get some feedback from @timdorr and others if its the right direction. Let me know if there are others I should ping once I feel like its in a good place.

[mattkrick]

Shouldn't this still be open? The package is not compatible with standard react lifecycle actions and there is no warning saying so...

[timdorr]

It's not this package alone. It's a combination of redux and react-redux, as you can create the loop I talked about with any old action.

I will add a warning about connecting location state, though. I can't think of a good reason to do that, and the router provides the same information to your route components already.

[timdorr]

Btw, onEnter does support async via an optional third param on your hook function. That's a done-style callback that you can use to resolve the async code. Just an FYI before I forget :)

[mattkrick]

@timdorr sounds good!
WRT the cb in onEnter, the onEnter hook refires once per dispatch, so unless you only dispatch something once inside an onEnter, you're left with a reeeaaaaally big headache.
Legacy code example (and a solution so delightfully hacky I'm almost proud of it) here: mattkrick/meatier@8cb3029#diff-c93753b501ff2b829707a764965759baL21

[mjrussell]

@timdorr @mattkrick I've created an auth wrapper library for redux here: https://github.com/mjrussell/redux-auth-wrapper

Its designed more in mind to connect to a reducer that holds the store data (rather than using the local storage) but I could probably work on making it support that example. (it might already do except for being able to subscribe to a local storage change)

It doesn't have any of the looping issues described in this ticket because it takes advantage of connect and the default pure option. Would love any feedback or input.

[mattkrick]

@mjrussell nice work, that was fast! Couple thoughts

• consider taking an object instead of 4 args for the wrapper
• consider using a webpack dev server to avoid using the file protocol in the example
• an inherent race condition exists between logging in & checking for auth. If i go straight to the Admin url, i'm gonna get kicked out before I can even send my LOGIN_REQUEST to the server. the component should either listen for user defined actions (eg LOGIN_SUCCESS & LOGIN_ERROR) or it should call a user-defined action creator (eg LOGIN_REQUEST) itself & asynchronously wait for the response.
• the example should be complex enough (eg use localStorage + a setTimeout) to showcase that the wrapper can mitigate the race condition stated above, currently I can't really test it since there is no login token stored to trigger a login attempt.

[timdorr]

@mattkrick Just to manage these issues better, let's move things over to issues on the repo itself.

[mjrussell]

Was just going to say Im going to port those there. 😄 Thanks @mattkrick