Add new API for react-router 6 by denysoblohin-okta · Pull Request #274 · okta/okta-react

denysoblohin-okta · 2024-02-19T16:08:49Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: OKTA-676780

Resolves #178

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

Reviewers

wip . . .

. . .

jaredperreault-okta · 2024-02-23T06:29:36Z

+    "react-dom": ">=16.8.0"
+  },
+  "optionalDependencies": {
    "react-router-dom": ">=5.1.0"


I think react-router-dom is actually an "optional peer dependency" (not an optional dependency)

https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependenciesmeta

Is there a minimum version of react-router-dom 6.x we should specify here?

I wanted to decouple okta-react from react-router.
See my comment: #274 (comment)
What if developer wants to use @remix-run/react router, or Next.js router, reach-router?
Then he would not want a react-router to be installed in node_modules.

I left react-router-dom in optionalDependencies just for legacy support of react-router 5

Understood, but a optional dependencies and a peer dependencies and a optional peer dependencies are not the same thing.

Optional dependency: A dependency that is may or may not be included (optional) to use a package. In the SIW we used to use a async library called fibers to improved the compile time of sass (https://github.com/okta/okta-signin-widget/pull/2863/files)

Peer dependency: A dependency which the application and library need a common version/instance of. For example okta-react need a common instance of React (and authjs). The application consuming okta-react needs React and okta-react needs React AND it needs to be the same "instance" of React

Optional Peer Dependency: Same as a peer dependency, however including the peer dependency is optional

Optional Peer Dependencies are defined as so:

"peerDependencies": { "@okta/okta-auth-js": "^7.x" }, "peerDependenciesMeta": { "@okta/okta-auth-js": { "optional": true } },

jaredperreault-okta · 2024-02-23T06:31:07Z

    ...Object.keys(pkg.peerDependencies || {}),
    ...Object.keys(pkg.dependencies || {}),
+    // for SecureRoute:
+    ...Object.keys(pkg.optionalDependencies || {}),


I don't think this is needed (see package.json comment)

jaredperreault-okta · 2024-02-23T06:32:54Z

+      "types": "./dist/bundles/types/react-router-5.d.ts",
+      "import": "./dist/bundles/okta-react-router-5.esm.js",
+      "require": "./dist/bundles/okta-react-router-5.cjs.js"
+    }


Can we export a react-router-6 bundle as well?

We could export a react-router-6 bundle with one component called like SecureOutlet that just renders <AuthRequired><Outlet /></AuthRequired>
Probably we should not name it SecureRoute as in RR5, because it's not a route.

jaredperreault-okta · 2024-02-23T06:37:15Z

-import LoginCallback from './LoginCallback';
-import Loading from './Loading';

+const useHashPathForLoginCalback = config.oidc.responseMode === 'fragment' || !config.oidc.pkce;


Can you rename this variable, use is usually reversed for hooks in React

Renamed to usingFragmentForLoginCalback

jaredperreault-okta · 2024-02-23T07:32:25Z

+): null => {
+  // We don't want any side effects (excess render, effect run) on `restoreOriginalUri` change,
+  //  so keep and use the latest value from the ref
+  const restoreOriginalUriRef = React.useRef(restoreOriginalUri);


Why is a ref needed here?

It's a safe way to eliminate Two custom restoreOriginalUri callbacks are detected warning issues.
When another instance of restoreOriginalUri is passed to Security, the effect would not be triggered.
Using empty dependencies array also works, but gives lint error after adding react-hooks/rules-of-hooks to ESlint config

Yes, but that linter is used to warn to you to the possibility of a memory leak, it does not mean there is one. It's correct to pass an empty array for this case (we only want the check to fire once on component mount)

The gen3 widget does this as well:
https://github.com/okta/okta-signin-widget/blob/master/src/v3/src/components/Widget/index.tsx#L147

jaredperreault-okta · 2024-02-23T07:35:19Z

+export const waitForAuthenticated = (
+  oktaAuth: OktaAuth
+): Promise<boolean> => {
+  return new Promise((resolve) => {


I think it makes more sense to use await oktaAuth.isAuthenticated() here, rather than the authState

oktaAuth.isAuthenticated() can't be used for this purpose (purpose is to use support react-router's loader).
oktaAuth.isAuthenticated() will return promise that will immediately be resolved with true/false rather that actually waiting for auth state with isAuthenticated: true

(Maybe it's better to delete waitForAuthenticated util if it's confusing, and not support loader)

oktaAuth.isAuthenticated() will return promise that will immediately be resolved with true/false rather that actually waiting for auth state with isAuthenticated: true

I'm not sure I follow this. oktaAuth.isAuthenticated will attempt to read the tokens from the tokenManager and determine their validity (and attempt a renew) before resolving

.

denysoblohin-okta · 2024-02-26T15:00:36Z

Closing in favour of #275

jaredperreault-okta · 2024-02-26T14:41:23Z

+    "react-dom": ">=16.8.0"
+  },
+  "optionalDependencies": {
    "react-router-dom": ">=5.1.0"


Understood, but a optional dependencies and a peer dependencies and a optional peer dependencies are not the same thing.

Optional dependency: A dependency that is may or may not be included (optional) to use a package. In the SIW we used to use a async library called fibers to improved the compile time of sass (https://github.com/okta/okta-signin-widget/pull/2863/files)

Peer dependency: A dependency which the application and library need a common version/instance of. For example okta-react need a common instance of React (and authjs). The application consuming okta-react needs React and okta-react needs React AND it needs to be the same "instance" of React

Optional Peer Dependency: Same as a peer dependency, however including the peer dependency is optional

Optional Peer Dependencies are defined as so:

"peerDependencies": { "@okta/okta-auth-js": "^7.x" }, "peerDependenciesMeta": { "@okta/okta-auth-js": { "optional": true } },

jaredperreault-okta · 2024-02-26T14:49:57Z

+
+  if (!isLoginRedirect) {
+    if (children) {
+      return (<>{children}</>);


We can provide samples for HashRouters, but I don't think we need direct API support. I don't think it's worth adding complexity to the API

jaredperreault-okta · 2024-02-26T14:52:05Z

  if (handleLoginError) {
-    return <ErrorReporter error={handleLoginError} />;
+    return <ErrorReporter error={handleLoginError} />
+  } else if (!authState?.isAuthenticated) {


nit: can you flip this boolean, so the default case renders <Loader />

else if (authState?.isAuthentcated) { return <ReactRouterDom.Route {....routeProps } /> } else { return Loading; }

jaredperreault-okta · 2024-02-26T14:53:43Z

+  loginError: Error | null;
+}
+
+const useAuthRequired = (


Yes, they will be similar, but I think the implementations will different slightly

jaredperreault-okta · 2024-02-26T14:58:02Z

+): null => {
+  // We don't want any side effects (excess render, effect run) on `restoreOriginalUri` change,
+  //  so keep and use the latest value from the ref
+  const restoreOriginalUriRef = React.useRef(restoreOriginalUri);


Yes, but that linter is used to warn to you to the possibility of a memory leak, it does not mean there is one. It's correct to pass an empty array for this case (we only want the check to fire once on component mount)

The gen3 widget does this as well:
https://github.com/okta/okta-signin-widget/blob/master/src/v3/src/components/Widget/index.tsx#L147

jaredperreault-okta · 2024-02-26T15:01:22Z

+export const waitForAuthenticated = (
+  oktaAuth: OktaAuth
+): Promise<boolean> => {
+  return new Promise((resolve) => {


oktaAuth.isAuthenticated() will return promise that will immediately be resolved with true/false rather that actually waiting for auth state with isAuthenticated: true

I'm not sure I follow this. oktaAuth.isAuthenticated will attempt to read the tokens from the tokenManager and determine their validity (and attempt a renew) before resolving

jaredperreault-okta · 2024-02-26T15:03:16Z

+  } else if (!isAuthenticated) {
+    return Loading;
+  } else {
+    return <>{children}</>;


I like your idea of calling this SecureOutlet, let's forward this that

However this component should not render children and should instead return <Outlet />

denysoblohin-okta added 24 commits February 14, 2024 19:36

wip

e821364

wip . . .

use Secure for reach sample

8e64efd

refactoring

71bdf3e

5 hash

3ef0c8a

utils

58a72b9

react-router-5

6e54e63

.

4b8a1d1

rr 5

c317f49

rr 6

c720629

rr 6 hash

8914d49

nit

a308d9d

upd other test apps

6960502

add sample rr6-data

0633a46

.

37ace82

test fixes

572cd1f

.

32e0102

. . .

fix

f39e794

nits

b50a031

.

7e886a1

.

952a2b7

.

b80dd17

.

ae234ca

chlog

86ddb97

optional dep

978ba66

denysoblohin-okta added 2 commits February 22, 2024 17:34

added tests

ccfd34b

nit

4093629

denysoblohin-okta marked this pull request as ready for review February 22, 2024 16:07

denysoblohin-okta requested a review from jaredperreault-okta February 22, 2024 16:07

jaredperreault-okta requested changes Feb 23, 2024

View reviewed changes

fix - revert Can't handle redirect

7ec4bcc

denysoblohin-okta force-pushed the od-router-OKTA-676780 branch from 3b39c86 to 7ec4bcc Compare February 23, 2024 15:34

denysoblohin-okta added 11 commits February 23, 2024 17:37

rename useHashPathForLoginCalback -> usingFragmentForLoginCalback

f6da2cb

SecureLayout

c486651

rename: Secure -> AuthRequired

4034d2c

Removed withLoginCallback

2b5d981

getComponents is not a hook

e6bb144

fix getComponents

3d27da2

remove test/jest/package.json

fa8779b

remove useLoginCallback

cadc88d

options -> props

d811d3a

remove requiresAuth. don't use hook in SecureRoute

7dc3b97

.

remove useLoginCallbackCheck

15c7c5b

denysoblohin-okta closed this Feb 26, 2024

jaredperreault-okta requested changes Feb 26, 2024

View reviewed changes

Conversation

denysoblohin-okta commented Feb 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denysoblohin-okta Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denysoblohin-okta commented Feb 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

denysoblohin-okta commented Feb 19, 2024 •

edited

Loading

denysoblohin-okta Feb 23, 2024 •

edited

Loading