London | 25-SDC-July | Andrei Filippov | Sprint 3 | Middleware exercises#24
London | 25-SDC-July | Andrei Filippov | Sprint 3 | Middleware exercises#24Droid-An wants to merge 6 commits intoCodeYourFuture:mainfrom
Conversation
Droid-An
added
Needs Review
LonMcGregor
added
the
Review in progress
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work on these, the logic makes sense, however I've spotted some areas you could make improvements
off-the-shelf-middleware/express.js
Outdated
| app.use(assignHeader); | ||
| app.use(express.json()); | ||
|
|
||
| // add `-H 'Content-Type: application/json'` to curl request |
There was a problem hiding this comment.
What does this comment mean? If this is a test case I would suggest moving it to a test file, rather than leaving it as a comment inline
There was a problem hiding this comment.
yeah, I agree. Since I don't have any tests in this repo, I decided to move it to the readme file because that comment is more like an instruction.
There was a problem hiding this comment.
Yes, that kind of comment is more suited to the readme. Good idea
custom-written-middleware/express.js
Outdated
| return; | ||
| } | ||
| if ( | ||
| typeof body != "object" || |
There was a problem hiding this comment.
If I pass a string like {"blah": 2}, this is a valid json string so it reaches this part of the code, but it crashes here because there's no .some() on an object, only arrays. Could you improve the input validation?
There was a problem hiding this comment.
Got it. The middleware now checks if the request body is an array. I missed in the requirements that we should reject requests when the POST body is not a JSON array.
I've also updated the error message accordingly.
There was a problem hiding this comment.
Great! This solution looks good now
LonMcGregor
added
Reviewed
Droid-An
added
Needs Review
|
Your PR couldn't be matched to an assignment in this module. Please check its title is in the correct format, and that you only have one PR per assignment. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
LonMcGregor
added
Complete
2 participants
Learners, PR Template
Self checklist
Changelist
written 2 middlewares, and used built-in one
Questions
no questions