Skip to content

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Nov 2, 2020

react-router v6 introduces breaking changes

  • useRouteMatch is not exported, see issue. react-router v6 uses useMatch instead
  • <Route>s should be inside <Routes>
  • <Route exact> removed, so need to use * in path if it's not exact
  • <Route component> and <Route render> removed, replaced with <Route element>

Issue: https://github.com/okta/okta-oidc-js/issues/853
Internal ref: https://oktainc.atlassian.net/browse/OKTA-318565

PR for samples: okta/samples-js-react#143

Warning! This PR uses react-router 6.0.0-beta.0 and should be revised after stable RR release. Do not merge.

@denysoblohin-okta denysoblohin-okta force-pushed the OKTA-318565-react-router-6 branch 2 times, most recently from de2226d to bc33c24 Compare November 10, 2020 15:36
<Routes>
<Route path='/login' element={<CustomLogin />}/>
<Route path='/sessionToken-login' element={<SessionTokenLogin />}/>
<SecureRoute path='/protected' element={<Protected />}/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to specify /protected/* here as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, just noticed it was exact before

@denysoblohin-okta denysoblohin-okta force-pushed the OKTA-318565-react-router-6 branch from 92b6178 to d03728b Compare November 18, 2020 00:32
@denysoblohin-okta denysoblohin-okta marked this pull request as ready for review November 18, 2020 00:51
@denysoblohin-okta denysoblohin-okta force-pushed the OKTA-318565-react-router-6 branch 2 times, most recently from 8ff98c4 to e8c73b1 Compare December 18, 2020 08:59
@@ -1,3 +1,13 @@
# 5.0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's base this PR against the dev-5.0 branch instead of master.

// Redirect to the /login page that has a CustomLoginComponent
// This example is specific to React-Router
history.push('/login');
navigate('/login');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no change here. navigate should only be a replacement of restoreOriginalUri from oktaAuth to decouple react-router dependency in <Security>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shuowu
Copy link
Contributor

shuowu commented Dec 23, 2020

@denysoblohin-okta Let's split react-router@6(beta) change and decoupling react-router from <Security> into two different PRs as we want to merge them separately.

For the decoupling PR, please base it against dev-5.0 branch instead of master.
For the react-router 6 PR, you can set it as a draft PR.

@denysoblohin-okta denysoblohin-okta marked this pull request as draft December 23, 2020 18:34
Copy link
Contributor Author

@denysoblohin-okta denysoblohin-okta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shuowu
I included adding navigate prop to Security in this PR to not break support of react-router 5.
react-router 5 should use useHistory hook, react-router 6 should use useNavigate hook.
So looks like this PR depends from decoupling PR.

// Redirect to the /login page that has a CustomLoginComponent
// This example is specific to React-Router
history.push('/login');
navigate('/login');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shuowu
Copy link
Contributor

shuowu commented Dec 23, 2020

I included adding navigate prop to Security in this PR to not break support of react-router 5.

Let's focus on the decoupling PR first (targeting to dev-5.0). Then get back to this PR when react-router 6 is ready.

As we don't know a clear timeline for when react-router@6 will be officially available, it may even come to okta-react v6 or later.

@denysoblohin-okta denysoblohin-okta force-pushed the OKTA-318565-react-router-6 branch from 8331378 to b75892c Compare January 15, 2021 16:31
@denysoblohin-okta denysoblohin-okta changed the base branch from master to dev-5.0 January 15, 2021 16:39
const { oktaAuth, authState, _onAuthRequired } = useOktaAuth();
const match = useRouteMatch(routeProps);
const { path, caseSensitive } = routeProps;
const match = path ? useMatch.call(null, { path, caseSensitive }) : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useMatch doesn't seem to be matching a route when basename property is set
we could provide useLocation().pathname value(which includes basename) as path parameter for useMatch()

@ericlifs
Copy link
Contributor

Is there a way we can merge this? React router now has a stable v6 release.

Thanks!

@flying-sheep
Copy link
Contributor

What’s the way forward here? Anything missing?

@difilippoagustin
Copy link

Is there any news with this?

@djrumph
Copy link

djrumph commented Dec 31, 2021

Wanted to follow up on this as well. Curious if this is planned to be merged in anytime soon.

@PakitoSec
Copy link

Could you please merge and release this pull requests ? we need it for our deployment...

@sturmery
Copy link

sturmery commented Jan 15, 2022

@denysoblohin-okta @oleksandrpravosudko-okta Please get this merged, it's really blocking our transition to react-router-dom v6.x. Thank you.

@dawit-nigusu
Copy link

dawit-nigusu commented Jan 24, 2022

@denysoblohin-okta @oleksandrpravosudko-okta
I am also stacked here, wanted to upgrate to react-router-dom v6 but okta/react seems dont support that because of useRouteMatch by useMatch . anytime soon this PR can be merged please?

@ondrejvelisek
Copy link

Hi there, anything new with this? May I somehow help to push it forward? Thanks.

@gvonderschmitt
Copy link

I have the same issue and I am stuck as well. This issue is important and urgent - altough react-router-dom v6 is not new anymore.
So please merge this into the main branch as soon as possbible as a lot of people are waiting for this. Many thx in advance.

@geoalexidis
Copy link

wen react-router v6?

@gvonderschmitt
Copy link

wen react-router v6?

Yes

@johnflux
Copy link

Is there any way to use this using npm?
Any idea when this will be merged please?

@johnflux
Copy link

Attempting to npm install the pull request fails with a typescript error:

npm install https://github.com/okta/okta-react#OKTA-318565-react-router-6

npm ERR! prepareGitDep [!] (plugin rpt2) Error: ~/.npm/_cacache/tmp/git-clone-de1a165a/src/SecureRoute.tsx(29,33): semantic error TS2339: Property 'call' does not exist on type '((_props: PathRouteProps | LayoutRouteProps | IndexRouteProps) => ReactElement<any, string | JSXElementConstructor<any>>) | ... 32 more ... | ForwardRefExoticComponent<...>'.
npm ERR! prepareGitDep   Property 'call' does not exist on type 'Context<NavigationContextObject>'.
npm ERR! prepareGitDep src/SecureRoute.tsx

@flying-sheep
Copy link
Contributor

flying-sheep commented Feb 15, 2022

I have a v6 only version here: https://github.com/flying-sheep/okta-react/tree/flying-sheep/okta-react-router-6

see here for the changes: master...flying-sheep:flying-sheep/okta-react-router-6

@uday-at-tomo
Copy link

This approach works for us with latest react and react router

#178 (comment)

@washingtonsoares
Copy link

Hi guys, any update on this? is there anything missing?

@yanavozniuk
Copy link

Any updates??

@seanmcquaid
Copy link

Any updates on this??

@yanavozniuk
Copy link

Any updates on this??

I created my own security route
And stop using it from okta

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

Successfully merging this pull request may close these issues.