-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
doc: clarify http error events after calling destroy() #46903
Conversation
Review requested:
|
`AbortController` will behave the same way as calling `.destroy()` on the | ||
request itself. | ||
request. Specifically, the `'error'` event will be emitted with an error with |
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.
Nit
request. Specifically, the `'error'` event will be emitted with an error with | |
request itself. Specifically, the `'error'` event will be emitted with an error with |
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.
Was going for brevity. "Itself" doesn't seem to add clarity. What do you think?
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 dont think its a necessary change to this pr, Id leave it as it was previously
request. Specifically, the `'error'` event will be emitted with an error with | ||
the message `'AbortError: The operation was aborted'`, the code `'ABORT_ERR'` |
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.
request. Specifically, the `'error'` event will be emitted with an error with | |
the message `'AbortError: The operation was aborted'`, the code `'ABORT_ERR'` | |
request. Specifically, the `'error'` event will be emitted with the message `'AbortError: The operation was aborted'`, the code `'ABORT_ERR'` |
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.
This parallels the wording in the bullet points above and makes it clear that the argument is an Error and not a string.
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.
It sounds a bit reduntant to me, feel free to ignore
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
Landed in f0ade70 |
PR-URL: #46903 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #46903 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #46903 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #46903 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Clarifies the errors after calling
destroy()
, as well as aborting a passed AbortController.Aside, the statement "Passing an
AbortSignal
and then callingabort()
on the correspondingAbortController
will behave the same way as calling.destroy()
" appears correct for Node.js v19, but incorrect for Node.js v16 (didn't test versions between those). Specifically, callingcontroller.abort()
after the'response'
event has no effect in v16.