-
Notifications
You must be signed in to change notification settings - Fork 2
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
Pagination & Filters #154
Pagination & Filters #154
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.
A lot of work went into this. I have a few things that should be kept in mind the next time the code's changed
.map((record) => record.get("suggested").properties) | ||
.slice(0, 15); | ||
return res.json({ status: "ok", users }); | ||
|
||
const users = allUsers.slice( |
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.
This code would still be slow if a million users registered. It's still taking all the users from the database.
I think it should be done in the database like this: MATCH (u:User) RETURN u LIMIT 16 SKIP n*16
. This way the database can cache the result and it should be really fast
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.
Hmm I never thought of this solution, thanks! I will try to fix this now :)
async (req: Request, res: Response) => { | ||
try { | ||
const session: Session = driver.session(); | ||
const userId: string = req.params.userId; | ||
const page: number = parseInt(req.params.page); | ||
const maxUsersOnPage = 3; | ||
const page: number = parseInt(req.query.page as 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.
Casting should be done if you are absolutely sure that the type can be safely cast.
In this case req.query.page
can be undefined
and parseInt()
returns NaN
.
The code still works because NaN
is falsy but I found it unintuitive.
It could be changed to parseInt(req.query.page || "")
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.
Should be fixed by now
@@ -90,13 +90,13 @@ friendshipRouter.get( | |||
); | |||
|
|||
friendshipRouter.get( | |||
"/:userId/friend-suggestions/:page", | |||
"/:userId/friend-suggestions", | |||
async (req: Request, res: Response) => { |
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.
Response isn't typed so type checking doesn't work
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 don't remember if it was already in code or I added, but changing the Response to custom would cause to change it everywhere where friends are mentioned, like friend-suggestions or friends endpoints, each to corresponing endpoint. I didn't come to any other conslusion, so if you have any, let me know! 😊
if (users.length === 0) { | ||
return res.status(404).json({ | ||
status: "not found", | ||
message: "No users found with given queries", |
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.
All other responses have a different format: { status: "error", errors: { "users": "not found" } }
.
In this case the page having no users isn't an error, the page is just empty
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.
Yes, you're right 😅
I tried to use different response codes, like 400 for bad requests and 404 for not found
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.
Pagination 🤌😃👍
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.
ok
Changes:
Tests will be fixed till the end of the day,