feat(node): Add shouldHandleError option to fastify error handler by AbhiPrasad · Pull Request #13198 · getsentry/sentry-javascript

AbhiPrasad · 2024-08-02T14:41:46Z

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

AbhiPrasad · 2024-08-02T14:41:46Z

resolves#13197

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

AbhiPrasad · 2024-08-02T14:41:46Z

resolves#13197

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

AbhiPrasad · 2024-08-02T14:41:46Z

resolves#13197

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

AbhiPrasad · 2024-08-02T14:41:46Z

resolves#13197

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

AbhiPrasad · 2024-08-02T14:41:46Z

resolves#13197

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

AbhiPrasad · 2024-08-02T14:41:46Z

resolves#13197

Aligns fastify error handler with the express one.

Adds shouldHandleError to allow users to configure if errors should be captured
Makes sure the default shouldHandleError only captures errors for 5xx status codes.

tmcw · 2024-08-05T18:40:22Z

+ const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
+
+ fastify.addHook('onError', async (request, reply, error) => {


I think, since digging into this more, that my initial suggestion was incomplete - reply.status is probably going to be 200 here, and instead there is, I think, error.statusCode, according to the fastify-sentry plugin? https://github.com/immobiliare/fastify-sentry/blob/main/lib/base.js#L43-L46

This is not a fastify-sentry plugin, but an immobiliare plugin for fastify. Not official.
Reply and Error are not the same since the reply could have been sent before the error occurred.

xr0master · 2024-09-09T06:17:43Z

+ * @param error Captured middleware error
+ */
+ // eslint-disable-next-line @typescript-eslint/no-explicit-any
+ shouldHandleError?(this: void, request: any, reply: any, error: Error): boolean;


It is better to use explicit types instead of any
import type { FastifyRequest, FastifyReply } from 'fastify';

for that we need to depend on the fastify package, which we cannot do in the generic node package.

// grumbling-mod-on
When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
// grumbling-mod-off
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Or the generic type.

When someone was too lazy to take the framework packages out of the general node package :) Although this was proposed for version 8
It was a conscious decision not to do this.
Perhaps, a dummy interface can be used. After all, only a little piece of it is used.
Which is exactly as fragile as any, if not more, since it gives a false sense of security and disables all of the ts-eslint checking.

I have to disagree with you. The any can completely disable checks, but the interface will force you as a developer to add/edit the necessary entry. This means that some commands will not be called accidentally. As below, when the statusCode may be undefined.
Anyway, this is just my opinion. However, the number of open issues with the new version 8 makes it clear that it is possible. In any case, it is your conscious decision.

I think you're making wrong assumptions about how our linting works. When we define stuff as any, yes typescript will not check it anymore, however, eslint will still not let us access any fields without narrowing down and checking for their exact type first.
I encourage you to pull the repo and try it out.

xr0master · 2024-09-09T06:28:53Z

+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+function defaultShouldHandleError(_request: any, reply: any, _error: Error): boolean {
+ // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+ return reply.statusCode >= 500;


statusCode can be undefined

// eslint-disable-next-line is used everywhere, it is not clear why you need eslint in the project then.

statusCode can be undefined
Could probably implement this similar to what's been done in the express integration.

xr0master · 2024-10-12T10:43:25Z

@magnusburton Hi, there might be some communication issue since I'm not working on it.
Just looked at the changes.

maxbeatty · 2024-10-23T03:53:57Z

@AbhiPrasad thanks for taking this on!

Is there anything I can do to help get this shipped?

maxbeatty · 2024-10-31T18:51:03Z

@lforst is there anything I can do to help get this merged? Thanks!

AbhiPrasad · 2024-11-04T18:39:56Z

I was just reminded by this PR - I had accidentally unsubscribed from it. Apologies about the delay, I'm pushing up changes now, let's get this to the finish line.

Sorry about the trouble everyone - totally my fault!

mydea · 2025-03-18T09:18:36Z

@AbhiPrasad can we get this merged? :D

AbhiPrasad · 2025-03-21T02:44:44Z

This got pretty stale, so opened #15771, added a test as well, as well as better docstrings.

Supercedes #13198resolves#13197 Aligns fastify error handler with the express one. 1. Adds `shouldHandleError` to allow users to configure if errors should be captured 2. Makes sure the default `shouldHandleError` does not capture errors for 4xx and 3xx status codes. ## Usage ```js setupFastifyErrorHandler(app, { shouldHandleError(_error, _request, reply) { return statusCode >= 500 || statusCode <= 399; }, }); ```

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

feat(node): Add shouldHandleError option to fastify error handler
0459800

AbhiPrasad requested review from a team and mydea August 2, 2024 14:41

AbhiPrasad self-assigned this Aug 2, 2024

AbhiPrasad requested review from lforst and removed request for a teamAugust 2, 2024 14:41

This was referenced Aug 2, 2024
Sentry reports handled errors in Fastify integration #13197
Closed
Filter errors based on 4xx status codes in Node #11720
Open

AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024

Merge branch 'develop' into abhi-fix-fastify-errors
6e4ee13

tmcw reviewed Aug 5, 2024
View reviewed changes

magnusburton approved these changes Sep 5, 2024
View reviewed changes

mydea approved these changes Sep 6, 2024
View reviewed changes

xr0master reviewed Sep 9, 2024
View reviewed changes

juliangruber mentioned this pull request Jan 28, 2025
Bad requests shouldn't create Sentry errors CheckerNetwork/spark-stats#299
Closed

AbhiPrasad mentioned this pull request Mar 21, 2025
feat(node): Add fastify shouldHandleError#15771
Merged

AbhiPrasad closed this Mar 21, 2025

AbhiPrasad deleted the abhi-fix-fastify-errors branch March 24, 2025 19:20

		const shouldHandleError = options?.shouldHandleError \|\| defaultShouldHandleError;

		fastify.addHook('onError', async (request, reply, error) => {

Uh oh!

Uh oh!

Conversation

AbhiPrasad commented Aug 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xr0master commented Oct 12, 2024

Uh oh!

maxbeatty commented Oct 23, 2024

Uh oh!

maxbeatty commented Oct 31, 2024

Uh oh!

AbhiPrasad commented Nov 4, 2024

Uh oh!

mydea commented Mar 18, 2025

Uh oh!

AbhiPrasad commented Mar 21, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Uh oh!

Conversation

AbhiPrasad commented Aug 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xr0master commented Oct 12, 2024

Uh oh!

maxbeatty commented Oct 23, 2024

Uh oh!

maxbeatty commented Oct 31, 2024

AbhiPrasad commented Mar 21, 2025•
edited
Loading

AbhiPrasad commented Mar 21, 2025•
edited
Loading