Allow timing out without erroring #30
Conversation
|
This would be unexpected as a default, but I'm willing to support it behind an option. |
|
Bump |
|
Did I miss anything? |
| reject(message); | ||
| } else { | ||
| const errorMessage = message ?? `Promise timed out after ${milliseconds} milliseconds`; | ||
| reject(new TimeoutError(errorMessage)); |
There was a problem hiding this comment.
Is it worth exporting this helper? I find this pattern in every place where a customizable error is desired:
reject(createError(`Promise timed out after ${milliseconds} milliseconds`, message))
There was a problem hiding this comment.
I haven't done that yet, I haven't thought of any API to specify both message and constructor. I'll publish it under the name as-error unless you have better suggestions. All "create-" and "cast-" options were taken because until recently Error subclassing was not a thing.
Side note: I think specifying a whole error instance isn't ideal, what do you think? Maybe it should be:
error: 'Custom Message' // uses default TimeoutError
// or
error: 'CustomConstructorError' // uses default message
// or
error: [CustomConstructorError, 'Custom Message'] // calls new CustomConstructorError('Custom Message')The alternative would be to reinstantiate error: new MyError('soup') via new error.constructor(error.message) but that might be unexpected?
There was a problem hiding this comment.
I'm considering releasing a more generic "error utils" package with https://github.com/refined-github/refined-github/blob/951f267db9441ae084ee660725d878bc1717bf5d/source/helpers/abort-controller.ts#L11 too. I saw p-timeout also has similar abort logic.
|
Any thoughts on this versus having a separate option? For example, |
| reject(message); | ||
| } else { | ||
| const errorMessage = message ?? `Promise timed out after ${milliseconds} milliseconds`; | ||
| reject(new TimeoutError(errorMessage)); |
|
Note: this is breaking + there's another breaking change awaiting us in a comment (Node 18 LTS) |
| Default: `'Promise timed out after 50 milliseconds'` | ||
|
|
||
| Specify a custom error message or error. | ||
| Specify a custom error message or error to throw when it times out: |
There was a problem hiding this comment.
This needs to be changed in the TS comment too.
There was a problem hiding this comment.
Now if only there was a way to automate this 😜
Done
|
Can you make it non-breaking? Just add a stub in the TS docs. I don't think it's worth doing a major release just for this. |
|
I thought you asked to rename the option? I can revert that change in this PR though, sure |
|
Yes, I forgot to say rename and keep compatibility. I can add the compatibility if you don't want to. |
No, the new name is an improvement. |
This reverts commit b5b18d3.
|
I left the rename out of this PR for now, I think it'd be easier to rename separately for tracking, documentation and discussion. The release notes can then point to the specific PR for that. I can work on that PR too |
|
The return type is now incorrect if |
|
I'm afraid I can't type that. It takes me a while to set up conditional returns 😳 Also I added a failing test for now |
Looks ok? ⬇️ |
|
Oh great! Tests pass so all good By the way why are there two return types (I'm not talking about "undefined")? |
Like we do in
element-ready, sometimes timeouts are not errors:https://github.com/sindresorhus/element-ready#timeout
Thoughts on this proposal before I continue?