Add Timeout to Finder #79
Conversation
finder.ts
Outdated
| @@ -20,6 +20,8 @@ export type Options = { | |||
| optimizedMinLength: number | |||
| threshold: number | |||
| maxNumberOfTries: number | |||
| timeoutMs: number | |||
There was a problem hiding this comment.
I see that the input to finder is using Partial but I think we can possibly use undefined for timeoutMs as well.
There was a problem hiding this comment.
Let me try that out! -1 is less straight-forward
finder.ts
Outdated
| @@ -42,6 +44,8 @@ export function finder(input: Element, options?: Partial<Options>) { | |||
| optimizedMinLength: 2, | |||
| threshold: 1000, | |||
| maxNumberOfTries: 10000, | |||
| timeoutMs: -1, | |||
There was a problem hiding this comment.
Here we may just want to set timeoutMs to undefined.
finder.ts
Outdated
| @@ -84,6 +88,10 @@ function bottomUpSearch( | |||
| let current: Element | null = input | |||
| let i = 0 | |||
| while (current) { | |||
| const elapsedTime = new Date().getTime() - config.start.getTime(); | |||
| if (config.timeoutMs !== -1 && elapsedTime > config.timeoutMs) { | |||
There was a problem hiding this comment.
And here we can check if config.timeoutMs is defined
finder.ts
Outdated
| @@ -42,6 +44,8 @@ export function finder(input: Element, options?: Partial<Options>) { | |||
| optimizedMinLength: 2, | |||
| threshold: 1000, | |||
| maxNumberOfTries: 10000, | |||
| timeoutMs: -1, | |||
| start: new Date(), | |||
There was a problem hiding this comment.
To me start seems more like an internal constant that does not need to be exposed through options. Can we set the start const globally?
| @@ -20,6 +20,8 @@ export type Options = { | |||
| optimizedMinLength: number | |||
| threshold: number | |||
| maxNumberOfTries: number | |||
| timeoutMs: number | |||
| start: Date | |||
| } | |||
|
|
|||
| let config: Options | |||
There was a problem hiding this comment.
I think we could have let start: Date | undefined here
There was a problem hiding this comment.
I took start out of options but I think you meant timeoutMs could be undefined
|
Cool! Will review it tonight. |
|
Looks good! But let's update documentation on the readme. |
@antonmedv Thank you! I updated the README. |
Addresses Issue #77
Adding an optional timeout in milliseconds to finder for cases where finder calls need to execute under a certain time threshold.
I tested this by temporarily updating L14 of tests/test.js to
function check(html, config = {timeoutMs: 1}) {and seeing that a few tests timed out: