-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor: makes session management a bit more readable #304
Refactor: makes session management a bit more readable #304
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great :)
The merge-base changed after approval.
Regarding N° 3 and your comment, you might want to look at ExpressJS error handling. |
web/frontend/src/index.tsx
Outdated
const response = await fetch(ENDPOINT_PERSONAL_INFO, req); | ||
let result; | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware ! This is a dangerous change.
The fetch
API returns a promise.
await
ing on a Promise can throw an exception if the Promise is rejected.
This needs to be in the try/catch block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to put the try/catch around the whole function. What do you think?
authorization: {}, | ||
}; | ||
} else { | ||
const txt = await response.text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the response did not return a 200, doesn't this always throw ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's neither 200 nor 401
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I meant is: isn't response.text()
going to throw before your throw new Error()
, when there's no HTTP body ? (i.e. no error information other than the HTTP status sent from the backend)
I'm not sure about how the .text()
works in the Fetch API, but according to the documentation and the standard it seems it reads the body. Does it succeed on an empty body ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pierluca It succeeds. I tested it.
My test: I restarted the backend, and refreshed the UI causing the backend to return 504. the frontend then displayed the intended message.
web/frontend/src/index.tsx
Outdated
if (res.status !== 200) { | ||
const txt = await res.text(); | ||
throw new Error(`unexpected status: ${res.status} - ${txt}`); | ||
if (response.status === 200) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you feel about a switch statement ? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it. Looks good 👍
Sonar seems to have a couple of good points |
try { | ||
fetchData(); | ||
} catch (e: any) { | ||
setContent(<Failed>{e.toString()}</Failed>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, be careful. Here you're changing the content of the page, I assume.
You shouldn't do that in an async context without verifying if the component is still loaded or not.
You might want to have a read
https://devtrium.com/posts/async-functions-useeffect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that is correct, but it already existed like this, so I wouldn't want to change it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But still I'm gonna take the advice for when I can apply am improvement confidently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'm just reviewing some of this code for the first time - so I'm pointing out issues as I see them. That doesn't mean I expect all the fixes in the same PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still a few issues, but nothing specifically belonging in this PR.
I think we can merge :)
The merge-base changed after approval.
The merge-base changed after approval.
@pierluca Github dismissed your review for some reason. |
there's multiple ports in the script, so it's better to clarify the type of port this is.
this commit does two things: 1. Renames the session variables to match update on the backend 2. Handles 401 which is now sent by the backend for unauthenticated users
2ba4455
to
01c4478
Compare
Kudos, SonarCloud Quality Gate passed! |
(Skip first two commits, they have already been merged in another PR - rebase avoided for other reasons).
Changes
for n. 3, I only apply that to one endpoint. In a following PR, I want apply this to all other endpoints, but I need to think of a smarter way to unify the behaviour between all endpoints that require authentication.