Adds handling of render and children to SecureRoute #872
Conversation
| const { authService, authState } = useOktaAuth(); | ||
| const history = useHistory(); | ||
|
|
||
| useEffect(() => { | ||
| // Make sure login process is not triggered when the app just start | ||
| // Start login if and only if app has decided it is not logged inn | ||
| if(!authState.isAuthenticated && !authState.isPending) { | ||
| const fromUri = history.createHref(history.location); |
There was a problem hiding this comment.
Does this line actually fall into the default case of setFromUri? If that's the case, I would remove the history dependency as it would be deprecated in react-routerv6.
There was a problem hiding this comment.
An interesting question - I believe this is all retaining the original code flow, which I also think predates the default setFromUri. I'll poke at a few cases, and if it is safe, pull out the useHistory - I'm certainly a fan of reducing dependencies, particularly in a useEffect() case.
| if(!authState.isAuthenticated && !authState.isPending) { | ||
| const fromUri = history.createHref(history.location); | ||
| authService.login(fromUri); | ||
| } | ||
| }, [authState, authService]); | ||
| }, [authState, authService, history]); |
There was a problem hiding this comment.
How about only depend on changes of authState.isAuthenticated and authState.isPending, then we can throttle the token only state update.
There was a problem hiding this comment.
That how it was, but then we run the risk of the history object being out of date.
There was a problem hiding this comment.
oh, I mean just replace authState. It can be an optimization, not mandatory.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, only "component" arguments to
<SecureRoute>actually enforced authenticationIssue Number: #808
What is the new behavior?
With this change,
<SecureRoute>adds formal support for "component", "render", and "children" just as<Route>from react-router-dom.Does this PR introduce a breaking change?
Note that the wrapping layers around a secured component now pull out all three potential params (component/render/children), which means not all three are passed through all the layers, which is for the best to avoid undetermined behavior with react-router, but we do have devs that have reported using such constructions.
This should follow the same priority order as react-router, but there is a risk of impact to those devs (though using render() was technically undocumented)
Other information
Reviewers