Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Server: Implement doSendEvents Fun #63

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Server: Implement doSendEvents Fun #63

merged 1 commit into from
Jun 7, 2024

Conversation

asmit27rai
Copy link
Contributor

@asmit27rai asmit27rai commented Jun 3, 2024 •
edited
Loading

Complete doSendEvents Fun

Fixes #61

Description

Work On doSendEvent Function that send event if response object is available.

Checklist

  • I have tested these changes locally.
  • I have reviewed the code and ensured it follows the project's coding guidelines.
  • I have updated the documentation, if necessary.
  • I have assigned reviewers to this pull request.

kuv2707
kuv2707 reviewed Jun 4, 2024
View reviewed changes
backend/eventRecipients.ts Outdated Show resolved Hide resolved
backend/eventRecipients.ts Outdated Show resolved Hide resolved
@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 5, 2024

@asmit27rai Please resolve the merge conflicts and also amend the commit message.
The fixes suggested in the review should not be a separate commit, neither should they be mentioned there.

The commit message should have a summary of the final changes that are there in the commit.
Squash the commits and enter a proper commit message. This would be good to go then.

@vercel Vercel
Copy link

vercel bot commented Jun 5, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
multiplayer-uno ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 7, 2024 5:04pm

@kuv2707 kuv2707 added under review There have been some rounds of review on the code changes in the PR. and removed can be merged soon labels Jun 6, 2024
@asmit27rai asmit27rai changed the title Server: Implement doSendEvent Fun Server: Implement doSendEvents Fun Jun 6, 2024
@asmit27rai
Copy link
Contributor Author

@asmit27rai Please resolve the merge conflicts and also amend the commit message. The fixes suggested in the review should not be a separate commit, neither should they be mentioned there.

The commit message should have a summary of the final changes that are there in the commit. Squash the commits and enter a proper commit message. This would be good to go then.

Conflicts Resolved.

@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 7, 2024

@asmit27rai That has introduced a merge commit.

Also the PR affects some unrelated files

@asmit27rai
Copy link
Contributor Author

@asmit27rai That has introduced a merge commit.

Also the PR affects some unrelated files

Done

kuv2707
kuv2707 reviewed Jun 7, 2024
View reviewed changes
backend/src/eventRecipients.ts
const events = eventQueue.get(clientId) || [];
if (res && events.length > 0) {
res.json({ events });
eventQueue.set(clientId, []);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also remove the clientId from clients since the current res object cannot be used further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also remove the clientId from clients since the current res object cannot be used further.

Done

@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 7, 2024

Please remove all other changes not related to the issue.

The PR is fit to be merged, once that is done

@sksmagr23 @asmit27rai
Server: Implement doSendEvents Function
85f9656 
The doSendEvents function has been added to eventRecipients.ts. This function sends the events array as the response body and clears the event queue after sending. Additionally, the client is removed from the clients map after events are sent.

Fixes: shivansh-bhatnagar18#61
@asmit27rai
Copy link
Contributor Author

Please remove all other changes not related to the issue.

The PR is fit to be merged, once that is done

Done.

@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 7, 2024

The commit message still isn't perfect, but I'm ignoring this time.
Please respect the commit message format.

@kuv2707 kuv2707 merged commit 4acd8fe into shivansh-bhatnagar18:master Jun 7, 2024
8 checks passed
@kuv2707
Copy link
Collaborator

kuv2707 commented Jun 7, 2024 •
edited
Loading

Merged, thanks @asmit27rai and @sksmagr23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kuv2707 kuv2707 kuv2707 left review comments

@AadityanshuSingh AadityanshuSingh AadityanshuSingh approved these changes

Assignees
No one assigned
Labels
under review There have been some rounds of review on the code changes in the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement the actual sending of events.
4 participants
@asmit27rai @kuv2707 @AadityanshuSingh @sksmagr23