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

Remounting Main Component #224

Open
rjardines opened this issue Feb 6, 2018 · 11 comments
Open

Remounting Main Component #224

rjardines opened this issue Feb 6, 2018 · 11 comments

Comments

@rjardines
Copy link

rjardines commented Feb 6, 2018

Hello,

I have a kind of master details page, something like:

/user
/user/m1
/user/m2

The authentication is defined to the main route user so every nested route must be authenticated, but every time the route change the user component is remount/

This is my setup

export const isUserAuthenticated = connectedRouterRedirect({
  // The url to redirect user to if they fail
  redirectPath: "/login",
  // Determine if the user is authenticated or not
  authenticatedSelector: state => state.user != null
  // A nice display name for this check
  wrapperDisplayName: "UserAuthenticated"
});
<Route path="/user" component={isUserAuthenticated (UserComponent)} />
<Route path="/user/m1" component={M1Component} />
<Route path="/user/m2" component={M2Component} />

Thanks in advance

Note: If I remove the authentication HOC like
<Route path="/user" component={UserComponent} />
everything works fine

@kusmierz
Copy link

same here, have you discover something new @rjardines?

@rjardines83
Copy link

Sadly no, I will post the solution i give to mu problem, maybe helps you

@dtweedle
Copy link

I can confirm that this is indeed a problem, any component that is mounted inside of the connectedRouterRedirect component is causing remounts every time some state update takes place.

Going to do some digging and see if I can see why.

@kusmierz
Copy link

@mjrussell do you have any idea why it remounts the component?

@dtweedle
Copy link

@kusmierz
@rjardines

After having played around and read through the source code, I think I have found the answer to this, which actually comes down to a bad example in the documentation the issue appears to be quite simple actually.

// BAD, Factory returns a new class on every single route match.

<Route
  path="protectedpath"
  component={isAuthenticated(YourProtectedComponent)}
/>

You should NOT put a function inside of the component prop, every time you match on this route the function is executed and you are returned a brand new constructor class which create it's own component instances.

What you actually want is this:

const checkAuthentication = isAuthenticated(YourProtectedComponent);

...

<Route 
  path="protectedpath"
  component={checkAuthentication}
/>

The reconciler can now see that the same constructor class is instantiating those components which prevents the application from remounting every single render.

This worked for me, I hope it works for you.

@mjrussell I recommend updating the docs to stop people putting the class factory inside of the component path. If you like I can do a PR.

@mjrussell
Copy link
Owner

Are you rendering the Route inside a component? You can apply the HOC in the component prop but only inside a ReactDOM.render( . I tried to explain this and list the safe vs non-safe ways in https://mjrussell.github.io/redux-auth-wrapper/docs/Getting-Started/Overview.html#where-to-apply but maybe I should make a note of it in the default example or make the default example use the application outside as you said.

@dtweedle
Copy link

dtweedle commented Mar 28, 2018

@mjrussell

I did indeed go over that section however the significance of it was lost on me at the time, having gone over those cases now I can see the error however I still think the documentation is confusing and doesn't highlight why similar looking but differently behaving use cases will fail.

It's not immediately obvious why something like this would work.

ReactDOM.render(
  <Provider store={store}>
    <Router history={history}>
      <Route path="/">
        <Route path="auth" component={authWrapper(Foo)}/>
        ...
      </Route>
    </Router>
  </Provider>,
  document.getElementById('mount')
)

And something like this would fail.

const App = () = {
  return <Route path="auth" component={authWrapper(Foo)}/>
}

ReactDOM.render(
  <Provider store={store}>
    <Router history={history}>
      <Route path="/">
        <App />
        ...
      </Route>
    </Router>
  </Provider>,
  document.getElementById('mount')
)

Other than more explicit documentation language, I'm not really sure how you can stop a user from mucking this up. Ideas?

Edit: By "fail" I'm referring to a remount on every single routing action undertaken.

@dtweedle
Copy link

@mjrussell

I think the best bet here is to edit/remove these two lines:

<Route path="profile" component={userIsAuthenticated(Profile)}/>
<Route path="login" component={userIsNotAuthenticated(Login)}/>

From this page:
https://mjrussell.github.io/redux-auth-wrapper/docs/Getting-Started/ReactRouter4.html

Or at least show them being consumed in a safe way, I think a lot of people like myself that tend to gloss over documentation saw this example and just blindly copied it (yes we probably shouldn't pander down to people but it might save you the hassle of constantly readdressing this issue).

It might also to make note in the FAQ about remounting cascades and that it's due to incorrectly consuming the API.

Cheers.

@matterai
Copy link

matterai commented Jul 2, 2018

Hey!

I am a new guy in React. And right now I am trying to set up authentication inside of my own app. I made everything like documentation says and ran though this issue. But it could not help me to solve my problem. I suppose it's quite close to discussion.

I have three routes in the app:
Landing: /
Dashboard (protected): /dashboard
Login page: /login

I have imported auth.js (with the same implementation as in docs but with my routes) to being able run this:

const Login = userIsNotAuthenticatedRedir(LoginComponent);
const Dashboard = userIsAuthenticatedRedir(DashboardComponent);

In my App class I have the next code:

  render() {
    return (
      <div className="container">
        <BrowserRouter>
          <div>
            <Header />
            <Route exact path="/" component={Landing} />
            <Route exact path="/dashboard" component={Dashboard} />
            <Route exact path="/login" component={Login} />
          </div>
        </BrowserRouter>
      </div>
    );

I have correct behaviour according redirects and everything looks nice. But, if I am logged in and I go to Login page, I see how it renders and after that it suddenly redirects me to dashboard with full page reload. What am I doing wrong? Or maybe I missunderstood something. I expect that I will not see page reload if redirect.

@dtweedle
Copy link

dtweedle commented Jul 3, 2018

@matterai

That seems unrelated to the the behaviour we are talking about above. It's more likely that you have a problem with the HOC's userIsNotAuthenticated and userIsAuthenticatedRedir.

This issue has nothing to do with redirect problems but to excessive remounts due to a reconciler issue.

Your problem is more likely due to your state changing in a way that you are either unaware of or incorrectly implementing.

@matterai
Copy link

matterai commented Jul 3, 2018

Thank you. I will continue investigation.

@mjrussell mjrussell pinned this issue Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants