-
-
Notifications
You must be signed in to change notification settings - Fork 25
London | 25-SDC-July | Mikiyas Gebremichal | Sprint 3 | Middleware exercises #34
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
base: main
Are you sure you want to change the base?
Conversation
LonMcGregor
left a comment
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.
Looking good, though I have a suggestion of something to think about to help improve the reusability of the middleware youve written
| return res.status(400).send("Request body must be a JSON array."); | ||
| } | ||
| // check every item is a string | ||
| const allStrings = parsed.every((item) => typeof item === "string"); |
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.
here you're combining json parsing and checking the input as strings. as middleware aims to allow re-use of code, what change could you make to this that would allow you to re-use these separate checks more easily?
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.
Thankyou for raising it. To solve this issue, i have refactored jsonArrayMiddleware into reusable middlewares for parsing and validation.
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 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.
@LonMcGregor let me know if i can close this PR thanks
LonMcGregor
left a comment
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 good to me
Learners, PR Template
Self checklist
Changelist
this is two express app with one containing custom middleware and another containing builtin middleware
Questions
No question