London | 26-Jul-SDC | Roman Sanaye | Sprint 2 | Chat App CYF-1162 - #114
London | 26-Jul-SDC | Roman Sanaye | Sprint 2 | Chat App CYF-1162#114RomanSanaye wants to merge 11 commits into
Conversation
RomanSanaye
added
the
Needs Review
LonMcGregor
left a comment
LonMcGregor
left a comment
There was a problem hiding this comment.
This app works well, you have added a like/dislike feature that works.
The code is well organised and easy to read.
I have a couple of questions to follow up on.
| <h1>Chat App</h1> | ||
| </header> | ||
|
|
||
| <main id="messages" class="messages"></main> |
There was a problem hiding this comment.
Good use of semantic HTML
| // Get messages from server | ||
| async function getMessages() { | ||
| const response = await fetch( | ||
| `https://x2fkdg4qtvw2zk6tfpgmud7g.trainees.hosting.cyf.academy/messages?since=${lastMessageId}`, |
There was a problem hiding this comment.
You repeat the live domain string in multiple different places. Imagine the domain changed - is there a way you could refactor the code so that changing this string would be easier?
There was a problem hiding this comment.
Hello @LonMcGregor ,
Thanks! I’ve moved the repeated domain into an API_URL constant, so it only needs to be changed in one place.
|
|
||
| // Send update to clients waiting for new messages | ||
| function notifyClients(update) { | ||
| while (callbacksForNewMessages.length > 0) { |
There was a problem hiding this comment.
Could you explain the approach you are using here for "callbacks" - how do you intend this to work?
There was a problem hiding this comment.
I’m using callbacks to implement long polling. When a client requests /messages and there are no new messages, I store a callback containing res.json() in callbacksForNewMessages instead of responding immediately. When a new message arrives, notifyClients(update) takes the stored callbacks and calls them with the update, which sends the new message to the waiting clients.
LonMcGregor
added
Reviewed
RomanSanaye
added
the
Needs Review
RomanSanaye
changed the title
|
Thanks for answering the questions. Using a constant is a better approach here, good idea. The task is complete now, good work |
LonMcGregor
added
Complete
|
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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
1 similar comment
|
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). If this PR needs reviewed, please add the 'Needs Review' label to this PR after you have resolved the issues listed above. |
2 participants
Self checklist
Task code
Changelist
This PR improves the chat application by adding client identification, message alignment, reactions, and long polling.
Changes made
clientIdusinglocalStorage.Deployed application
Frontend:
https://0l7lqpbmvhm5ffxxppby1k4f.trainees.hosting.cyf.academy/
Backend:
https://x2fkdg4qtvw2zk6tfpgmud7g.trainees.hosting.cyf.academy/