Repository navigation
London | 26-SDC-July | Raihan Sharif | Sprint 2 | Chat-app - #124
RaihanSharif wants to merge 52 commits into
Conversation
Can post message, with validation. Can get all messages.
Only return list of messages, or a single message. Clean up js doc
Messages can now be fetched or created using REST API. Client can then fetch subsequent messages using event streaming.
create new message handling function.
Websocket version only has websocket code, everything else is in a separate file that can be reused by polling version.
Extract common code for the web socket and polling versions.
|
Update: I have now created the two separate apps, one polling and one Websocket. Both try to reuse as much code as possible, both in the frontend and backend. Backend:
Frontend:
Application uses HTTP post to create new messages and reactions and event streaming through either long polling or websocket to read new events. Has some functionality to avoid race conditions by using even cursor per client. |
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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. |
LonMcGregor
left a comment
There was a problem hiding this comment.
Good work building this app, you've organised the files neatly and used classes where appropriate. I can also see places where you've given consideration to how you could extend with new functionality later.
I have spotted a few areas where you could improve, or where you could answer my questions to check your understanding of the code.
| const msg_body = document.getElementById("message-input").value; | ||
|
|
||
| try { | ||
| await chatRequest(`${BACKEND_URL}/messages`, { |
There was a problem hiding this comment.
If you are using the socket based app, does sendMessage have any way of using the socket, or does it end up making extra requests?
There was a problem hiding this comment.
In my current implementation the sendMessage function just uses HTTP. This was a conscious decision to keep thing simpler. Following my previous Event Driven Architecture, in which POST requests only create/update resources, and send a confirmation, but the actual fetching of new events is done through a separate event broadcasting either through polling or WebSocket.
I could implement a version which uses WebSocket, and add the necessary protocol/schema to indicate new message send and acknowledgement.
| @@ -0,0 +1,17 @@ | |||
| export class Message { | |||
| static #nextId = 0; | |||
There was a problem hiding this comment.
Can you explain what the static keyword here means?
There was a problem hiding this comment.
This is a class field, not an instance field. It belongs to the class itself rather than any one individual instance of the message class.
| /** | ||
| * Sends new events for each waiting request, if there are any events to send. | ||
| */ | ||
| notifyWaiters() { |
There was a problem hiding this comment.
What is the difference between a "waiter" and a "subscriber"?
There was a problem hiding this comment.
In this code waiters are the polling requests, and subscribers are WebSocket connections.
I was going to create a more general "subscriber" interface so that both WebSocket and HTTP, but I didn't have the time to get it done.
I've added some comments to make this clearer.
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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. |
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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. |
|
The files changed in this PR don't match what is expected for this task. Please check that you committed the right files for the task, and that there are no accidentally committed files from other sprints. Please review the 'files changed' tab at the top of the page. Here is an example of a file that has been changed on this branch but shouldn't be: 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. |
Learners, PR Template
Self checklist
Task code
CYF-1162
Changelist
Created a live updating chat app that allows client to use either long polling or short polling.
Users can add new message
User is notified whenever a new message is created by another user
User can like or dislike a message
User is notified when a message is liked/disliked
backend: https://z4k2yzxetkpevkwf6zy9ea37.trainees.hosting.cyf.academy/messages
frontend: https://qhdyqohwxffxraktvwh1e0ys.trainees.hosting.cyf.academy/