LONDON | 25-SDC-Nov | Jesus del Moral | Sprint 3 | Middleware #62
Conversation
delmorallopez
added
the
Needs Review
OracPrime
added
the
Review in progress
OracPrime
left a comment
OracPrime
left a comment
There was a problem hiding this comment.
A couple of comments to understand but no changes required
| */ | ||
| const usernameMiddleware = (req, res, next) => { | ||
| const username = req.header("X-Username"); | ||
| req.username = username ? username : null; |
There was a problem hiding this comment.
This code works, but it would probably be more normal to see either
req.username = username ?? null;
or
req.username = username || null;
A useful exercise is to understand the difference between the two and why the first one is better
There was a problem hiding this comment.
I understand the defferent
Fisrt one is better option because:
Safer, more explicit, less error-prone, modern JavaScript best practice
There was a problem hiding this comment.
It depends whether 0 and/or an empty string are valid usernames as well!
| let rawBody = ""; | ||
|
|
||
| req.on("data", chunk => { | ||
| rawBody += chunk; |
There was a problem hiding this comment.
This is good use of the req.on model, although other trainees just went for app.use(express.text()) !
There was a problem hiding this comment.
I can see this option
- Express middleware parses the request body automatically
- Converts the body into a string and stores it in req.body
- Less code, easier to use for trainees
There was a problem hiding this comment.
You do have to be careful though - unless you use text with type="application/json" it will just ignore the body!
OracPrime
added
Reviewed
2 participants
I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide