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

docs(client-aborted): remove deprecated function #4898

Merged
merged 1 commit into from
Jul 14, 2023
Merged

docs(client-aborted): remove deprecated function #4898

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions docs/Guides/Detecting-When-Clients-Abort.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const app = Fastify({

app.addHook('onRequest', async (request, reply) => {
request.raw.on('close', () => {
if (request.raw.aborted) {
if (request.raw.destroyed) {
Copy link
Member

Choose a reason for hiding this comment

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

I bit late for the comment, but destroyed is not identical to aborted.
destoryed should always true when calling request.destroy(). However, it doesn't indicate the request is aborted (meaning readableEnded is not true or complete is not true.)

https://github.com/nodejs/node/blob/f9737b1b8c96e43cedfff764634524df62cf3109/lib/_http_incoming.js#L222-225

Copy link
Member

Choose a reason for hiding this comment

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

It seems is deprecated but still we use it for the onRequestAbort hook (ref)

Maybe we should change the docs to onRequestAbort as best-effort?

Copy link
Member

Choose a reason for hiding this comment

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

It seems is deprecated but still we use it for the onRequestAbort hook

Yes, it is deprecated and we are still using.
But I assume it would never been able to remove.

Maybe we should change the docs to onRequestAbort as best-effort?

The guide is explaining request abort in detail. I would keep it since the function is reference this document for further explanation.

app.log.info('request closed')
}
})
Expand Down Expand Up @@ -85,7 +85,7 @@ functionality:
of `{ ok: true }`.
- An onRequest hook that triggers when every request is received.
- Logic that triggers in the hook when the request is closed.
- Logging that occurs when the closed request property `aborted` is true.
- Logging that occurs when the closed request property `destroyed` is true.

In the request close event, you should examine the diff between a successful
request and one aborted by the client to determine the best property for your
Expand All @@ -97,7 +97,7 @@ You can also perform this logic outside of a hook, directly in a specific route.
```js
app.get('/', async (request, reply) => {
request.raw.on('close', () => {
if (request.raw.aborted) {
if (request.raw.destroyed) {
app.log.info('request closed')
}
})
Expand All @@ -112,7 +112,7 @@ aborted and perform alternative actions.
```js
app.get('/', async (request, reply) => {
await sleep(3000)
if (request.raw.aborted) {
if (request.raw.destroyed) {
// do something here
}
await sleep(3000)
Expand Down