Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Option to throw on error status codes #1453

Merged
merged 8 commits into from
May 20, 2022
Merged

feat: Option to throw on error status codes #1453

merged 8 commits into from
May 20, 2022

Conversation

kibertoad
Copy link
Contributor

refs #1205

kibertoad
kibertoad commented May 18, 2022
View reviewed changes
docs/api/Dispatcher.md

#### 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was always passed, but neither used nor documented

@codecov-commenter
Copy link

codecov-commenter commented May 18, 2022 •
edited
Loading

Codecov Report

Merging #1453 (d3d6aa0) into main (324beb6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            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
Impacted Files Coverage Δ
lib/api/api-request.js 100.00% <100.00%> (ø)
lib/core/errors.js 100.00% <100.00%> (ø)
lib/core/request.js 98.66% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 324beb6...d3d6aa0. Read the comment docs.

@ronag
Copy link
Member

ronag commented May 18, 2022

I think this is outside of the scope of what undici should be doing.

@kibertoad
Copy link
Contributor Author

@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.

@kibertoad
Copy link
Contributor Author

@mcollina Could use your perspective here as well.

mcollina
mcollina reviewed May 19, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor for this change too.

test/client.js
signal,
path: '/',
method: 'GET',
throwOnError: true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a better name for the option, in this example is not throwing at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any ideas? What aspects would you like emphasized? Axios uses validateStatus for similar config, got uses throwHttpErrors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronag any preference on the name?

types/dispatcher.d.ts Show resolved Hide resolved
@ronag
Copy link
Member

ronag commented May 19, 2022

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?

@kibertoad
Copy link
Contributor Author

@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?

@kibertoad
Copy link
Contributor Author

@ronag I gave it a shot, please let me know if revised error structure is what you wanted.

ronag
ronag reviewed May 20, 2022
View reviewed changes
lib/api/api-request.js Outdated Show resolved Hide resolved
ronag
ronag reviewed May 20, 2022
View reviewed changes
lib/core/request.js Outdated Show resolved Hide resolved
kibertoad and others added 2 commits May 20, 2022 10:12
@kibertoad @ronag
make check explicit
b36f767 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@kibertoad @ronag
make condition more explicit
d3d6aa0 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@kibertoad kibertoad closed this May 20, 2022
@kibertoad kibertoad reopened this May 20, 2022
mcollina
mcollina approved these changes May 20, 2022
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 1f2cca6 into nodejs:main May 20, 2022
@kibertoad kibertoad deleted the feat/1205-error-handling branch May 20, 2022 11:04
@kibertoad
Copy link
Contributor Author

@mcollina What is the release cadence? Any estimates on when new version can be published?

@kibertoad
Copy link
Contributor Author

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.

@mcollina
Copy link
Member

thanks, once that's stable I'll ship a release.

KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
@kibertoad @ronag @KhafraDev
feat: Option to throw on error status codes (nodejs#1453)
5a41ba5 
* 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>
@mdashlw mdashlw mentioned this pull request Nov 4, 2022
Closed
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
@kibertoad @ronag @metcoder95
feat: Option to throw on error status codes (nodejs#1453)
0be4178 
* 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>
@xconverge xconverge mentioned this pull request Jan 26, 2023
Closed
@s97712
Copy link

s97712 commented Aug 19, 2023

This will lose the error stack.

@kibertoad
Copy link
Contributor Author

@s97712 can you elaborate? there is no error stack on error code replies per se

@s97712
Copy link

s97712 commented Aug 19, 2023

If I use throwOnError to throw an error, I will get the following stack trace.
This will be difficult to pinpoint the exact location of the error.

  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

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
@kibertoad @ronag @crysmags
feat: Option to throw on error status codes (nodejs#1453)
881e6a1 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kibertoad @codecov-commenter @ronag @mcollina @s97712