MF-56: Add logout to primary nav bar #5
Conversation
src/root.component.tsx
Outdated
| const [isLoggedIn, setIsLoggedIn] = React.useState(true); | ||
|
|
||
| React.useEffect(() => { | ||
| authResource.getLoggedInUser().subscribe(({ person }) => { |
There was a problem hiding this comment.
We should store the subscription and unsubscribe during the effect's cleanup function. See https://reactjs.org/docs/hooks-effect.html#effects-with-cleanup
const subscription = authResource.getLoggedInUser().subscribe(...)
return () => subscription.unsubscribe()
There was a problem hiding this comment.
Oops I keep forgetting to do that
src/root.component.tsx
Outdated
| .logoutUser() | ||
| .then(() => setIsLoggedIn(false)) | ||
| .catch(err => { | ||
| throw err; |
There was a problem hiding this comment.
.catch(err => {throw err}) is a no-op.
Calling .catch changes the promise from error to resolved status. But rethrowing in the callback sets it back into error status. This will cause an "unhandled promise rejection" error, which is not a good thing because not all browsers have a way of capturing and recording those.
It's good to be thinking about error handling, but we haven't established the patterns yet and this is not the right pattern. For now, you could delete this .catch() or you could do the following, which is closer to what the error handling will be:
.catch(err => {
// setTimeout makes this error thrown to window instead of just changing
// the promise's status.
setTimeout(() => {
throw err
})
})
src/root.component.tsx
Outdated
| <div className={styles.avatar}>AB</div> | ||
| </nav> | ||
| <BrowserRouter> | ||
| <React.Fragment> |
There was a problem hiding this comment.
nit: you can do <> instead of <React.Fragment> for all use cases except when you're adding a key prop to the fragment.
src/auth.resource.tsx
Outdated
| logoutUser | ||
| }; | ||
|
|
||
| function getLoggedInUser() { |
There was a problem hiding this comment.
is this function worth it? Maybe we just use getCurrentUser directly inside of the component?
|
Hi @joeldenning, I updated this PR, however I noticed something strange. Sending a DELETE request doesn't log out the user so the user ends up being redirected to the home page yet again |
src/root.component.tsx
Outdated
| @@ -14,29 +32,65 @@ function Root() { | |||
| }; | |||
| }, [sidenavOpen]); | |||
|
|
|||
| function logout() { | |||
There was a problem hiding this comment.
nit: performing API calls inside of a click handler isn't usually best practice -- better to have an Effect handle the api request, and call setIsLoggingOut(true) inside of the click handler.
Reason is that effects get cleaned up when the component unmounts, but click handlers don't. Also that having an isLoggingOut boolean state property can be helpful in disabling other functionality while we're waiting for the logout to happen.
src/root.component.tsx
Outdated
| }).then(() => setIsLoggedIn(false)); | ||
| } | ||
|
|
||
| function getOpenmrsSpaBase() { |
There was a problem hiding this comment.
This function seems rather useless. Did you create it so you could @ts-ignore outside of the JSX?
See this issue for how you can use ts-ignore in JSX. The syntax is quite odd, but works.
{/*
// @ts-ignore */}
{!isLoggedIn && <Redirect to={`${getOpenmrsSpaBase()}login`} />} Or:
{!isLoggedIn && <
// @ts-ignore
Redirect to={`${getOpenmrsSpaBase()}login`} />
}
src/root.component.tsx
Outdated
| function logout() { | ||
| return openmrsFetch("/ws/rest/v1/session", { | ||
| method: "DELETE" | ||
| }).then(() => setIsLoggedIn(false)); |
There was a problem hiding this comment.
we're not handling errors on this again. Andrei and I will be working on the error handling stuff this week so we'll have better answers for how to handle the errors then. So your code here is fine for now. But just wanted to point out that it's always best to handle promise rejections.
|
@fatmali , did you follow up on the delete request failing to log out the user? I had noticed something similar when trying to do so myself manually (from console). |
|
@jonathandick @joeldenning Hitting DELETE now works as expected, also updated this PR with the changes requested. |
There was a problem hiding this comment.
Build failed. Looks like you committed some files without letting the prettier pre-commit hook run. Specifically the root.component.tsx file
No description provided.