Skip to content
This repository has been archived by the owner on Oct 24, 2024. It is now read-only.

Component unmounts when using SecureRoute #777

Closed
2 of 9 tasks
christopherhuii opened this issue May 15, 2020 · 17 comments
Closed
2 of 9 tasks

Component unmounts when using SecureRoute #777

christopherhuii opened this issue May 15, 2020 · 17 comments

Comments

@christopherhuii
Copy link

christopherhuii commented May 15, 2020

In my application, I've noticed that state is lost when switching between matching react router routes when using SecureRoute.

This previously wasn't an issue in v.1.2.3, but I've noticed it in both v.2.0.1 and v.3.0.1

react-router-dom is v5.1.2

I'm submitting this issue for the package(s):

  • jwt-verifier
  • okta-angular
  • oidc-middleware
  • okta-react
  • okta-react-native
  • okta-vue

I'm submitting a:

  • Bug report
  • Feature request
  • Other (Describe below)

Current behavior

The mounted component unmounts when React Router params updated

Expected behavior

I would expect the component to stay mounted and not lose state

Minimal reproduction of the problem with instructions

👇 This is the App with its routes. Notice the Unprotected view uses Route from react-router-dom and Protected view uses SecureRoute from @okta/okta-react.

    <div className="App">
      <BrowserRouter>
        <Security>
          <Switch>
            <Route exact path="/" component={Home} />
            <Route path="/login" component={Login} />
            <Route path="/unprotected/:id" component={Unprotected} />
            <SecureRoute path="/protected/:id" component={Protected} />
            <Route path="/implicit/callback" component={LoginCallback} />
          </Switch>
        </Security >
      </BrowserRouter>
    </div>

👇 This is the Unprotected view. This functions as expected. As a user, I can increment the counter and when I click Next Page the state is persisted because the route still matches.

const Unprotected = () => {
  const history = useHistory();
  const { id } = useParams();
  const [counter, setCounter] = useState(0);
  return (
    <div>
      <h1>Unprotected</h1>
      <p>{`Current Count: ${counter}`}</p>
      <button onClick={() => setCounter((prev) => prev + 1)}>
        Add to Counter
      </button>
      <button onClick={() => history.push(`/unprotected/${parseInt(id) + 1}`)}>
        Next Page
      </button>
    </div>
  );
};

👇 This is the Protected view. As a user, I can increment the counter BUT when I click Next Page the state in the counter is wiped out. I'm guessing this has to do with SecureRoute causing the view to unmount; i'm not certain though.

const Protected = () => {
  const history = useHistory();
  const { id } = useParams();
  const [counter, setCounter] = useState(0);
  return (
    <div>
      <h1>Protected</h1>
      <p>{`Current Count: ${counter}`}</p>
      <button onClick={() => setCounter((prev) => prev + 1)}>
        Add to Counter
      </button>
      <button onClick={() => history.push(`/protected/${parseInt(id) + 1}`)}>
        Next Page
      </button>
    </div>
  );
};

Extra information about the use case/user story you are trying to implement

This was NOT an issue in v1.2.3, but we noticed this issue in both v.2.0.1 and the latest v.3.0.1.

Environment

  • Package Version: 3.0.1
  • Browser: Google Chrome Version 81.0.4044.129
  • OS: macOS Catalina Version 10.15.4
  • Node version (node -v): 12.10.0
@AlexeyBarkow
Copy link

I experienced same issue. This should help you:

<SecureRoute 
  render={({ component }) => component()} 
  component={Protected} 
  path="/protected/:id"
/>

@swiftone
Copy link
Contributor

Internal ref: OKTA-288357

@jchabotamica
Copy link

i believe this has to do with the fact that SecureRoute now uses the useHistory hook. If you do a history.push in your child component, this may cause issues when your component is a child of SecureRoute

@CodeWaala
Copy link

Any update on the above issue ?

@CodeWaala
Copy link

render={({ component }) => component()}

If I use this solution. this.props.match becomes undefined. Are you able to read 'id' value ?

@aarongranick-okta
Copy link
Contributor

@CodeWaala Are you using the most recent version of okta-react (3.0.2)?

@CodeWaala
Copy link

CodeWaala commented Jun 30, 2020

@CodeWaala Are you using the most recent version of okta-react (3.0.2)?

Yes. I upgraded to 3.0.2, this fixed the issue regarding the props missing the 'match' (#724) but this issue still exists.

@jeremyhermann
Copy link

We are hitting this issue as well. Any progress on a fix?

@jchabotamica
Copy link

jchabotamica commented Jul 20, 2020

I experienced same issue. This should help you:

<SecureRoute 
  render={({ component }) => component()} 
  component={Protected} 
  path="/protected/:id"
/>

This interesting as it seems to help, but according to react router documentation:
Warning: <Route component> takes precedence over <Route render> so don’t use both in the same .

Is okta-react doing something different with this?

@pvdstel
Copy link

pvdstel commented Jul 30, 2020

For what it's worth, we've worked around this problem by creating a functionally similar component with a fix. This exact problem can be fixed by modifying these lines to the following.

const WrappedComponent = useMemo(() => {
    const PassedComponent = component || function() { return null; };
    return (wrappedProps) => (<RequireAuth><PassedComponent {...wrappedProps}/></RequireAuth>);
}, [component]);

What changed here is that useMemo is now used to cache the wrapped component. This causes React to not treat the component as a completely new component on each render. I've confirmed that it works by modifying the built code to add that useMemo.

There are other issues with SecureRoute in its current form that aren't fixed by this change. While there are some PRs to resolve that, it just takes (too) long for an official resolution.

@sarahdayan
Copy link

I experienced the same issue, @AlexeyBarkow's solution fixed it.

Note that SecureRoute's component prop must be passed as a reference for it to work.

This won't work (SecureRoute will unmount):

<SecureRoute
  render={({ component }) => component()}
  path="/protected/:id"
  component={() => (
    <Wrapper>
      <Protected />
    </Wrapper>
  )}
/>;

But this will:

function WrappedProtected() {
  return (
    <Wrapper>
      <Protected />
    </Wrapper>
  );
}

<SecureRoute
  render={({ component }) => component()}
  path="/protected/:id"
  component={WrappedProtected}
/>;

If you're nesting SecureRoutes (e.g., to protect your entire application), you need to apply the same fix on the parent SecureRoute:

<SecureRoute
  render={({ component }) => component()}
  path="/"
  component={AppWithRoutes}
/>;

@swiftone
Copy link
Contributor

I was digging into this issue, but I've been unable to reproduce it with recent versions of okta-react (and react-router-dom 5.2.0/react-router 16.13.1)

Here's what I tried:

// App.jsx
const App = () => (
  <Router>
    <Security { ...config }>
      <Route path="/" exact component={Home} />
      <Route path="/implicit/callback" component={LoginCallback} />
      <SecureRoute path="/openpagecount/:id" component={PageCount}/>
      <Route path="/closedpagecount/:id" component={PageCount}/>
    </Security>
  </Router>
);
export default App;

// PageCount.jsx
import React, { useState, useEffect } from 'react';
import { useHistory, useParams } from 'react-router-dom';

const PageCount = () => {
  const history = useHistory();
  const { id } = useParams();
  const [counter, setCounter] = useState(0);

  useEffect(() => {
    console.log('mounting');
    return () => console.log('unmounting!')
  }, [])

  return (
    <div>
      <p>Current Count: {counter}</p>
      <button onClick={() => setCounter( prev => prev + 1 )}>
        Add to Counter
      </button>
      <button onClick={() => history.push(`/openpagecount/${parseInt(id) + 1}`)}>
        Next Page (Unprotected)
      </button>
      <button onClick={() => history.push(`/closedpagecount/${parseInt(id) + 1}`)}>
        Next Page (Protected)
      </button>
    </div>
  );
};
export default PageCount;

Let us know if you're continuing to see the unmounting issue, and if so, with what versions (including react-router).

@chinanderm
Copy link

chinanderm commented Aug 23, 2020

@swiftone I am experiencing the problem with react-router-dom 5.2.0. Below is my set up. MySecureApp is remounted on every route change. @AlexeyBarkow's solution works.

<Router>
   <Security
      {...myOktaConfig}
   >
      <Switch>
         <Route path="/login" exact component={Login} />
         <Route path="/implicit/callback" component={LoginCallback} />
         <SecureRoute path="/" component={MySecureApp} />
      </Switch>
   </Security>
</Router>



const MySecureApp = () => {

   useEffect(() => {
      console.log("mounted")
   }, [])

   return (
      <>
         <Route exact path="/" component={Home} />
         <Route exact path="/page1" component={Page1} />
         <Route exact path="/page2" component={Page2} />
      </>
   )

}

@swiftone
Copy link
Contributor

@chinanderm - interesting, thanks for the case. We are rewriting SecureRoute( see #872 ) as the current version was never written to support all the <Route> options. As such, I'd recommend against violating react-router-dom standards by having both component AND render, but I'll this case against the new code before we push it out.

@swiftone
Copy link
Contributor

@chinanderm - I've confirmed your case doesn't unmount/remount using the new code.

With that cleared, I'll start our internal CI process to get that version (3.0.5) of okta-react published.

@chinanderm
Copy link

@chinanderm - I've confirmed your case doesn't unmount/remount using the new code.

With that cleared, I'll start our internal CI process to get that version (3.0.5) of okta-react published.

Thank you! Will upgrade as soon as that's out.

@swiftone
Copy link
Contributor

@chinanderm - okta-react 3.0.5 has been released

Closing this ticket as this release should close out the last of the ways to trigger this bug. If anyone still has issues with SecureRoute unmounting with okta-react 3.0.5+, please open a new ticket with details and reference this one.

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

No branches or pull requests

10 participants