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

When continued variable is explicitly false, delay is set to be 250ms #27

Closed
wants to merge 0 commits into from

Conversation

abaevbog
Copy link
Contributor

@abaevbog abaevbog commented May 5, 2023

Addresses Issue # 130

When a message has continued=false, delay is only 250ms

@abaevbog
Copy link
Contributor Author

abaevbog commented May 5, 2023

I had a very hard time running it because it seems like uws package is not distributed through npm anymore and there was a mismatch between my node module api and the one wanted by the last available uws package

server.js Outdated
continuedTimeouts[topic] = setTimeout(fn, config.get('noContinuedDelay'));
} else {
continuedTimeouts[topic] = setTimeout(fn, config.get('continuedDelayDefault'));
}
Copy link
Member

Choose a reason for hiding this comment

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

House style: Newline between } and else

(Reasoning is that it allows for more logical commenting of an entire else block)

@@ -30,6 +30,7 @@ var config = {
// Notification action period -- clients are given a randomly chosen delay within this time
// period before they should act upon the notification, so that we don't DDoS ourselves
globalTopicsDelayPeriod: 180 * 1000,
noContinuedDelay: 250,
Copy link
Member

Choose a reason for hiding this comment

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

noContinuedDelay is a little confusing, since Continued is still passed in that case.

How about continuedDelay (30s), notContinuedDelay (250ms), and defaultDelay (3s)?

@abaevbog
Copy link
Contributor Author

abaevbog commented May 8, 2023

Moved this to a PR request from feature branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants