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
feat: make hover evaluation timeout configurable #1243
Conversation
src/configuration.ts
Outdated
@@ -146,7 +146,7 @@ export interface IBaseConfiguration extends IMandatedConfiguration { | |||
/** | |||
* Timeouts for several operations (currently only source-maps) | |||
*/ | |||
timeouts: Partial<SourceMapTimeouts>; | |||
timeouts: Partial<SourceMapTimeouts> & HoverEvaluationTimeout; |
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.
I think we can instead rename SourceMapTimeouts
and add hoverEvaluation
to it
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.
Thought about this already, but I wasn't sure, whether I should move the definition of SourceMapTimeouts
then.
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.
The SourceMapTimeouts
type is used in the Sources
class. If the type is modified & renamed, anywhere in Sources
where SourceMapTimeouts
is currently used, the hoverEvaluation
timeout needs to have been set, although Sources
although Sources
is completely unrelated to it and therefor never uses it.
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.
I pushed a commit where SourceMapTimeouts
still is a separate interface, but the types are structured a little bit better.
private getHoverEvalTimeout() { | ||
const configuredTimeout = this.launchConfig.timeouts?.hoverEvaluation; | ||
if (configuredTimeout === undefined) { | ||
return 10 * 1000; |
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.
Btw, TS (and recent JS) allows separators so you can write this as 10_000
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.
Lgtm, thanks!
Fixes #1242