-
Notifications
You must be signed in to change notification settings - Fork 265
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
fix(extensions): make sure periodical full ui reload doesn't happen #6693
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: GLEF1X <glebgar567@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR
FYI this PR being more on technical debt issue will be looked/discussed/merged after 1.10 release
extensionApi.env.createTelemetryLogger().logError(err.toString()); | ||
} | ||
}); | ||
void monitorDaemon(extensionContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hello, here it looks you're bypassing the linter and do not catch error on promises, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
monitorDaemon
doesn't return a promise
extensionApi.env.createTelemetryLogger().logError(err.toString()); | ||
function monitorDaemon(extensionContext: extensionApi.ExtensionContext): void { | ||
const loop = (): NodeJS.Timeout => | ||
// eslint-disable-next-line @typescript-eslint/no-misused-promises |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not ignore the misused-promises in the project
// Start monitoring by calling passed in constructor parameters function until stopLoopPredicate returns false | ||
start(): void { | ||
const loop = (): NodeJS.Timeout => | ||
// eslint-disable-next-line @typescript-eslint/no-misused-promises |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be reused among extensions, because it is common pattern throughout:
- run callback on constant intervals,
- do not fail on error in callback just report it or allow to configure error handling caback,
- provide predicate function for exit from monitoring loop
Monitoring loop can be implemented as (see timers/promises example
import { setInterval } from 'node:timers/promises';
for await (const _interval of setInterval(interval)) {
try {
await callback(extensionContext);
} catch (error) {
errorhandler?(error)
}
if (stopLoop)
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or use npm package like 'set-interval-async' that would handle some other usecases
// Dynamic strategy.
import { setIntervalAsync, clearIntervalAsync } from 'set-interval-async/dynamic';
const timer = setIntervalAsync(async () => {
// ...
if (shouldStop) {
await clearIntervalAsync(timer);
}
try {
await callback()
} catch(err) {
handleError(err);
}
}, interval);
Signed-off-by: GLEF1X glebgar567@gmail.com
What does this PR do?
Attempts to resolve problems stated in #6663(full UI reload because the nodejs stack is overflowing)
Screenshot / video of UI
What issues does this PR fix or reference?
#6663
How to test this PR?