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

"Two custom restoreOriginalUri callbacks are detected" warning #227

Open
CruseCtrl opened this issue Apr 27, 2022 · 17 comments · Fixed by #275
Open

"Two custom restoreOriginalUri callbacks are detected" warning #227

CruseCtrl opened this issue Apr 27, 2022 · 17 comments · Fixed by #275
Labels
bug Something isn't working

Comments

@CruseCtrl
Copy link

CruseCtrl commented Apr 27, 2022

Describe the bug?

I'm getting a warning saying that "Two custom restoreOriginalUri callbacks are detected. The one from the OktaAuth configuration will be overridden by the provided restoreOriginalUri prop from the Security component."

What is expected to happen?

I shouldn't get a warning

What is the actual behavior?

I get a warning

Reproduction Steps?

const oktaAuth = new OktaAuth({})

type Props = { children: React.ReactNode }

export const OktaSecurity = ({ children }: Props) => {
  const navigate = useNavigate()

  const restoreOriginalUri = useCallback(
    (oktaAuth: OktaAuth, originalUri: string) => {
      navigate(toRelativeUrl(originalUri || '/', window.location.origin), {
        replace: true,
      })
    },
    [navigate],
  )

  return (
    <Security oktaAuth={oktaAuth} restoreOriginalUri={restoreOriginalUri}>
      {children}
    </Security>
  )
}

SDK Versions

@okta/okta-auth-js@6.3.0
@okta/okta-react@6.4.3

Execution Environment

MacOS Big Sur 11.6

Additional Information?

It looks like this is caused in Security.tsx on lines 51-56.

In the first render, oktaAuth.options.restoreOriginalUri won't exist and so it won't log a warning. But then on line 54 it sets a value to oktaAuth.options.restoreOriginalUri. So on the next render when it checks if (oktaAuth.options.restoreOriginalUri && restoreOriginalUri) it will show the warning, because it's just added a value! (And the useEffect will run again because you've just changed one of the properties of oktaAuth.)

This problem is also mentioned in #170, where the accepted solution was to create a new oktaAuth for each render. This will get rid of the error because it doesn't reuse the oktaAuth which now has a restoreOriginalUri, but it's bad practice to create a new object on every render.

It's a bit weird that you're modifying the oktaAuth object that's passed in. Some options of how to fix this are:

  1. Create a copy of oktaAuth and modify the copy, leaving the original unchanged
  2. Pass the restoreOriginalUri into the OktaContext.Provider so that we can keep the original oktaAuth
  3. Remove the option to pass restoreOriginalUri as a separate parameter to Security, instead requiring that the user sets the restoreOriginalUri directly on the oktaAuth.options themselves (and add a suitable error message if the user hasn't done this)
  4. Or just remove this warning entirely, since it's broken

I think 3 is my favourite, but it would require a breaking change so I'd welcome any feedback. I'm also happy to raise a pull request once we've agreed upon a solution

@CruseCtrl CruseCtrl added the bug Something isn't working label Apr 27, 2022
@CruseCtrl CruseCtrl changed the title "Security.tsx:54 Two custom restoreOriginalUri callbacks are detected" warning "Two custom restoreOriginalUri callbacks are detected" warning Apr 27, 2022
@RikkiMasters
Copy link

Same issue since upgrading to React 18

@denysoblohin-okta
Copy link
Contributor

@CruseCtrl What are the reasons of re-rendering component?

@CruseCtrl
Copy link
Author

@denysoblohin-okta every time anything on the page changes, React re-renders the components. I'm not sure I fully understand the question

@denysoblohin-okta
Copy link
Contributor

Security component should re-render and call its useEffect hook if restoreOriginalUri or oktaAith changes.
oktaAuth should not change as it's being constructed outside of OktaSecurity.
So seems like restoreOriginalUri changes, probably due to change of navigate. Is it correct?
You can use React DevTools to investigate.

@CruseCtrl
Copy link
Author

CruseCtrl commented May 19, 2022

I'm not changing restoreOriginalUri. The Okta code is changing the value of restoreOriginalUri on the oktaAuth object that I'm passing in on line 54 of Security.tsx.

Edit: To be clear, the restoreOriginalUri prop that I'm passing into <Security restoreOriginalUri={restoreOriginalUri} /> isn't changing. The only thing that is changing is the restoreOriginalUri on oktaAuth.options.restoreOriginalUri, which is being changed by the Okta Security component

@denysoblohin-okta
Copy link
Contributor

Unlike react-router v5, react-router v6 will re-render router on each location change.
react-router v6 will change navigate function (returned from useNavigate hook) after each location change.
See https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/hooks.tsx#L190
Thus callbacks passed to <Security> (restoreOriginalUri and onAuthRequired) will change too, if memoized with dependency on navigate.
This leads to unnecessary side effects like multiple calls to useEffect of <Security>

It's safe to remove dependency from navigate.

  const restoreOriginalUri = useCallback(
    (oktaAuth: OktaAuth, originalUri: string) => {
      navigate(toRelativeUrl(originalUri || '/', window.location.origin), {
        replace: true,
      })
    },
    [],
  )

Some explanation of why it's safe:
In fact, useNavigate callback depends on several contexts - navigation context, route context and location context.
Location context value change should not affect on navigation to originalUri because it's absolute (starts with /).
And navigation context value (navigator, basepath) should not change in real application.

@denysoblohin-okta
Copy link
Contributor

Internal ref: OKTA-500147

@vdzerakh
Copy link

Unlike react-router v5, react-router v6 will re-render router on each location change. react-router v6 will change navigate function (returned from useNavigate hook) after each location change. See https://github.com/remix-run/react-router/blob/main/packages/react-router/lib/hooks.tsx#L190 Thus callbacks passed to <Security> (restoreOriginalUri and onAuthRequired) will change too, if memoized with dependency on navigate. This leads to unnecessary side effects like multiple calls to useEffect of <Security>

It's safe to remove dependency from navigate.

  const restoreOriginalUri = useCallback(
    (oktaAuth: OktaAuth, originalUri: string) => {
      navigate(toRelativeUrl(originalUri || '/', window.location.origin), {
        replace: true,
      })
    },
    [],
  )

Some explanation of why it's safe: In fact, useNavigate callback depends on several contexts - navigation context, route context and location context. Location context value change should not affect on navigation to originalUri because it's absolute (starts with /). And navigation context value (navigator, basepath) should not change in real application.

Hi, even though I've added a useCallback to restoreOriginalUri, and removed navigate from dependencies, the warning in the console still crops up.
The creating of oktaAuth is located in the main index.ts file

Do you have any ideas why the recommendations don't work?

@RikkiMasters
Copy link

I fixed it by disabling React strict mode. (Remove <React.StrictMode> from index.js)

Strict mode will intentionally render every component twice.

@ryanmr
Copy link

ryanmr commented Jun 16, 2022

I noticed the same thing as vdzerakh mentioned above. Even with the useCallback around the restore function, and without the navigate in the dependnency list, the warning still comes back up. With React 18 and Strict Mode, these future docs are incomplete. Disabling StrictMode isn't ideal.

I think the problem here is that Strict Mode enforces double renders in development so that you can catch side effects.

  React.useEffect(() => {
    if (!oktaAuth || !restoreOriginalUri) {
      return;
    }

    // Add default restoreOriginalUri callback
    if (oktaAuth.options.restoreOriginalUri && restoreOriginalUri) {
      console.warn('Two custom restoreOriginalUri callbacks are detected. The one from the OktaAuth configuration will be overridden by the provided restoreOriginalUri prop from the Security component.');
    }
    oktaAuth.options.restoreOriginalUri = (async (oktaAuth: unknown, originalUri: string) => {
      restoreOriginalUri(oktaAuth as OktaAuth, originalUri);
    }) as ((oktaAuth: OktaAuth, originalUri?: string) => Promise<void>);

  }, [oktaAuth, restoreOriginalUri]);

This useEffect triggers the first time, and the warning does not trigger, since the condition isn't true yet. But then on the second render (the one from Strict Mode), it triggers because the condition oktaAuth.options.restoreOriginalUri && restoreOriginalUri is true now, from having been set one useEffect prior, oktaAuth.options.restoreOriginalUri =.

I'm still thinking about alternative approaches to this.

Follow up

I did some rudimentary testing, and I came up with this:

return () => {
      oktaAuth.options.restoreOriginalUri = undefined;
};

This is a clean up function from the return of the useEffect. Whenever the <Security> re-renders now, either because its parent changes, its props change, or its internal state change, the useEffect will run, and so will this cleanup. When the cleanup runs, it sets the options.restoreOriginalUri back to undefined, so that on the next render the warning conditions are not true. Unfortunately, this does not fix the case where a user supplies an options.restoreOriginalUri callback in the OktaAuth config. I think that is OK given that with okta-react and how the <Security> component blindly overwrites the options.restoreOriginalUri anyway, the prop is the source of truth.

@kellengreen
Copy link

kellengreen commented Jun 27, 2022

Temp solution I'm using in the meantime:

import { Security } from '@okta/okta-react'
import { useCallback, useEffect, useRef } from 'react'
import { useNavigate } from 'react-router-dom'
import { toRelativeUrl } from '@okta/okta-auth-js'

export default function Root() {
  const navigate = useNavigate()
  const navigateRef = useRef(navigate)

  // Allow for "stale" navigate references since originalUri will be an absolute URL.
  const restoreOriginalUri = useCallback((_, originalUri = '/') => {
    const url = toRelativeUrl(originalUri, globalThis.location.origin)
    navigateRef.current(url)
  }, [])

  useEffect(() => {
    return () => {
      oktaAuth.options.restoreOriginalUri = undefined
    }
  }, [])

  return (
    <Security oktaAuth={oktaAuth} restoreOriginalUri={restoreOriginalUri}>
      ...
    </Security>
  )
}

@pzi
Copy link

pzi commented Jul 25, 2022

#239 seems to fix this.

@kellengreen
Copy link

kellengreen commented Jul 25, 2022

If I remove the above fix:

useEffect(() => {
  return () => {
    oktaAuth.options.restoreOriginalUri = undefined
  }
}, [])

I'm still seeing this error with @okta/okta-react=6.6.0, @okta/okta-auth-js=6.7.2, and react-router-dom=6.3.0:

Two custom restoreOriginalUri callbacks are detected. The one from the OktaAuth configuration will be overridden by the provided restoreOriginalUri prop from the Security component.

(anonymous) @ Security.tsx:54
commitHookEffectListMount @ react-dom.development.js:23150
invokePassiveEffectMountInDEV @ react-dom.development.js:25154
invokeEffectsInDev @ react-dom.development.js:27351
commitDoubleInvokeEffectsInDEV @ react-dom.development.js:27330
flushPassiveEffectsImpl @ react-dom.development.js:27056
flushPassiveEffects @ react-dom.development.js:26984
performSyncWorkOnRoot @ react-dom.development.js:26076
flushSyncCallbacks @ react-dom.development.js:12042
commitRootImpl @ react-dom.development.js:26959
commitRoot @ react-dom.development.js:26682
finishConcurrentRender @ react-dom.development.js:25981
performConcurrentWorkOnRoot @ react-dom.development.js:25809
workLoop @ scheduler.development.js:266
flushWork @ scheduler.development.js:239
performWorkUntilDeadline @ scheduler.development.js:533

I've also updated my "SecureRoute" to reflect the useEffect changes made in 6.6.0

export default function SecureRoute() {
  const { oktaAuth, authState } = useOktaAuth()

  useEffect(() => {
    if (authState?.isAuthenticated === false) {
      const originalUri = toRelativeUrl(
        window.location.href,
        window.location.origin,
      )
      oktaAuth.setOriginalUri(originalUri)
      oktaAuth.signInWithRedirect()
    }
  }, [oktaAuth, authState?.isAuthenticated])

  if (authState?.isAuthenticated === true) {
    return <Outlet />
  } else {
    return <FullPageLoading />
  }
}

@sridharan-santhanam
Copy link

@kellengreen @ryanmr - Thanks for the solutions.

This was linked to pull requests Feb 21, 2024
@phani1585
Copy link

@kellengreen / @ryanmr , do you have any idea whats need to be done in the react app when we doing the single sign out from the okta dashboard

@jaredperreault-okta
Copy link
Contributor

@phani1585 Can you please create a new issue, your question doesn't seem relevant to this thread

@kellengreen
Copy link

kellengreen commented Mar 13, 2024

@phani1585 sorry I'm not sure if I understand your question.

For what it's worth, I don't recall seeing this warning in recent releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants