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.

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

Instant notifications #53

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ringtailsoftware
Copy link
Contributor

Added an eventemitter to notify /poll that a notification has occurred.
/poll now stalls forever waiting for an event (once an event occurs, app.js repolls after 1s)
Adding a notification sends the event

@benbrown
Copy link
Owner

Rad. This is rad. Thanks.

@benbrown
Copy link
Owner

I am testing this right now and it is mind boggling how fast it is. I will have to implement this for the DM interface too ;p

@benbrown
Copy link
Owner

ah, one small bug - if there are unread messages already waiting, they won't show up til the first new incoming. the first request should always return current status.

@benbrown
Copy link
Owner

Also, not sure the impact of this - but the server-side listener continues to exist even if i navigate away from the page.

@ringtailsoftware
Copy link
Contributor Author

I think that fixes both issues

@benbrown
Copy link
Owner

The last few commits on here raise eyebrows - I don't really want to fire notifications for every single incoming post or dm. This will create a lot of noise in the notifications that has to be filtered out.

Instead, you could directly trigger the "check for notifications" event from those places, which would cause the longpoll to do the actual calculations...

… UserEvent directly - causing the long poll to end and the messages counts to be read.
@benbrown benbrown added the wont-merge indicates this PR will not be merged label Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wont-merge indicates this PR will not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants