feat: Option to throw on error status codes #1453
Conversation
|
|
||
| #### Parameter: `DispatchHandler` | ||
|
|
||
| * **onConnect** `(abort: () => void, context: object) => void` - Invoked before request is dispatched on socket. May be invoked multiple times when a request is retried when the request at the head of the pipeline fails. | ||
| * **onError** `(error: Error) => void` - Invoked when an error has occurred. May not throw. | ||
| * **onUpgrade** `(statusCode: number, headers: Buffer[], socket: Duplex) => void` (optional) - Invoked when request is upgraded. Required if `DispatchOptions.upgrade` is defined or `DispatchOptions.method === 'CONNECT'`. | ||
| * **onHeaders** `(statusCode: number, headers: Buffer[], resume: () => void) => boolean` - Invoked when statusCode and headers have been received. May be invoked multiple times due to 1xx informational headers. Not required for `upgrade` requests. | ||
| * **onHeaders** `(statusCode: number, headers: Buffer[], resume: () => void, statusText: string) => boolean` - Invoked when statusCode and headers have been received. May be invoked multiple times due to 1xx informational headers. Not required for `upgrade` requests. |
There was a problem hiding this comment.
this was always passed, but neither used nor documented
Codecov Report
@@ Coverage Diff @@
## main #1453 +/- ##
==========================================
+ Coverage 94.42% 94.44% +0.01%
==========================================
Files 49 49
Lines 4271 4284 +13
==========================================
+ Hits 4033 4046 +13
Misses 238 238
Continue to review full report at Codecov.
|
|
I think this is outside of the scope of what undici should be doing. |
|
@ronag What is the downside? It is a very simple piece of logic that significantly improves developer experience. Reimplementing it for every project is wasteful boilerplate. |
|
@mcollina Could use your perspective here as well. |
| signal, | ||
| path: '/', | ||
| method: 'GET', | ||
| throwOnError: true |
There was a problem hiding this comment.
We need a better name for the option, in this example is not throwing at all.
There was a problem hiding this comment.
Any ideas? What aspects would you like emphasized? Axios uses validateStatus for similar config, got uses throwHttpErrors.
There was a problem hiding this comment.
@ronag any preference on the name?
|
I'm on the losing end again 😢 ... can we at least make the error signature match that of https://www.npmjs.com/package/http-errors? |
|
@ronag Hopefully everyone will win after this feature is properly refined. Can you elaborate on what changes would you like to be made on error structure? I followed structure of other Undici errors, should this one be different for some reason? |
|
@ronag I gave it a shot, please let me know if revised error structure is what you wanted. |
Co-authored-by: Robert Nagy <ronagy@icloud.com>
Co-authored-by: Robert Nagy <ronagy@icloud.com>
|
@mcollina What is the release cadence? Any estimates on when new version can be published? |
|
I think some flakiness for Windows/Node 16 build was introduced again with this PR. I'll try to fiddle with the new tests added here, helped in the past. |
|
thanks, once that's stable I'll ship a release. |
* Improve coverage * Update TS types * Fix linting * Improve naming, add type tests * Address code review comments * make check explicit Co-authored-by: Robert Nagy <ronagy@icloud.com> * make condition more explicit Co-authored-by: Robert Nagy <ronagy@icloud.com> Co-authored-by: Robert Nagy <ronagy@icloud.com>
* Improve coverage * Update TS types * Fix linting * Improve naming, add type tests * Address code review comments * make check explicit Co-authored-by: Robert Nagy <ronagy@icloud.com> * make condition more explicit Co-authored-by: Robert Nagy <ronagy@icloud.com> Co-authored-by: Robert Nagy <ronagy@icloud.com>
|
This will lose the error stack. |
|
@s97712 can you elaborate? there is no error stack on error code replies per se |
|
If I use throwOnError to throw an error, I will get the following stack trace. ResponseStatusCodeError: Bad Request
- util.js:36 getResolveErrorBodyCallback
[admin]/[undici@5.23.0]/[undici]/lib/api/util.js:36:34
- task_queues:95 processTicksAndRejections
node:internal/process/task_queues:95:5
|
* Improve coverage * Update TS types * Fix linting * Improve naming, add type tests * Address code review comments * make check explicit Co-authored-by: Robert Nagy <ronagy@icloud.com> * make condition more explicit Co-authored-by: Robert Nagy <ronagy@icloud.com> Co-authored-by: Robert Nagy <ronagy@icloud.com>
refs #1205