add isFastifyError #86

Uzlopak · 2022-11-02T12:08:58Z

Closes #17

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

index.js

…ue, fix typings

Uzlopak · 2022-11-02T12:17:40Z

@jsumners

Should i change

fastify-error/index.js

Line 39 in d61f38e

FastifyError.prototype[Symbol.toStringTag] = 'Error'

to

FastifyError.prototype[Symbol.toStringTag] = 'FastifyError'

?

Uzlopak · 2022-11-02T12:20:53Z

I guess that is what you want.

Probably should be then a semver major.

Uzlopak · 2022-11-02T12:22:17Z

index.js

@@ -36,7 +36,7 @@ function createError (code, message, statusCode = 500, Base = Error) {

    this.statusCode = statusCode || undefined
  }
-  FastifyError.prototype[Symbol.toStringTag] = 'Error'
+  FastifyError.prototype[Symbol.toStringTag] = 'FastifyError'


Potentially breaking change and should result in a major versoin bump

mcollina · 2022-11-02T12:35:08Z

index.js

@@ -47,4 +47,11 @@ function createError (code, message, statusCode = 500, Base = Error) {
  return FastifyError
 }

+function isFastifyError (value) {
+  return Object.prototype.toString.call(value) === '[object FastifyError]'


I'm hardly -1 on this approach.

@mcollina I believe you want to say: "I'm strongly -1". Saying "I'm hardly" means "I'm in favor of this".

What is your disagreement with this? An instanceof check would break if someone is using their own import of fastify-error to check an error generated by Fastify's internal import of fastify-error.

@mcollina @jsumners suggested this approach. What should we agree on?

Would it work with a custom symbol?

No, the toStringTag is a spec defined symbol.

We can just set our custom
Symbol for every instance created by the constructor and check for it. We already do this in our codebase.

True. But we are delving deeper into hack territory.

Uzlopak · 2022-11-02T13:04:06Z

What about the classic?

function isFastifyError (value) {
  return (
    typeof value === 'object' &&
    value !== null &&
    value.name === 'FastifyError'
  )
}

I mean... if somebody changes the name of the error it is basically saying it is not a FastifyError anymore. Like we change the name from Error to FastifyError to indicate we have our own Error Class.

mcollina · 2022-11-02T13:59:59Z

Going back to the original issue: what problem are we trying to solve for here? What usage do we want? I do not see a good case to use that method that's not a code smell. At this point instanceof does not worsen the coupling.

The fact that the whole Fastify ecosystem could be built without this existing should tell something.

jsumners · 2022-11-02T14:01:27Z

Going back to the original issue: what problem are we trying to solve for here?

🤷‍♂️ I only have the recommendation to prevent "not an instance of" errors.

mcollina · 2022-11-02T14:01:32Z

My best recommendation would be to have a utility to check if it has a code and/or statusCode set.

Uzlopak · 2022-11-02T17:44:05Z

The question is if we should instead improve the documentation what we consider best practise and about ducktyping FastifyError.

I could do that instead, if we agree on

Eomm · 2022-11-02T17:51:13Z

index.js

@@ -47,4 +47,11 @@ function createError (code, message, statusCode = 500, Base = Error) {
  return FastifyError
 }

+function isFastifyError (value) {
+  return Object.prototype.toString.call(value) === '[object FastifyError]'


Would it work with a custom symbol?

Eomm · 2022-11-02T17:51:48Z

test/index.test.js

+  t.plan(8)
+
+  const NewError = createError('CODE', 'Not available')
+  t.equal(isFastifyError(NewError()), true)


I wonder if we should consider the createError instances as "application error" instead of Fastify Error

fox1t · 2022-11-02T18:16:19Z

This is easy as adding a custom unique symbol to every instance created by the constructor and then checking for it inside the isFasitfyError function.
@mcollina having this function will allow the users to easier check inside the error handlers if it is an application-level error or an unexpected unhandled error.

Uzlopak · 2022-11-02T18:38:15Z

I guess you could do

const kFastifyError = Symbol.for('FastifyError')

And add it like we do with toStringTag.

mcollina · 2022-11-02T20:29:11Z

@mcollina having this function will allow the users to easier check inside the error handlers if it is an application-level error or an unexpected unhandled error.

I typically use this module for my applications too.

The only way to know if an error comes from Fastify is to check the prefix of the code property.

jsumners · 2022-11-02T20:30:48Z

@mcollina having this function will allow the users to easier check inside the error handlers if it is an application-level error or an unexpected unhandled error.

I typically use this module for my applications too.

The only way to know if an error comes from Fastify is to check the prefix of the code property.

So your objection is the object name?

mcollina · 2022-11-02T21:44:34Z

My objection is on how this is supposed to be used. I would instanceof individual errors, but not a blanket check against all the errors this module can produce.

If somebody wants all Fastify core errors, they can do err.code?.indexOf('FST_') === 0.

jsumners · 2022-11-02T22:53:21Z

I would instanceof individual errors

This is not reliable. See the attached example. Within it is an example/00_main/index.js that imports some dependent library example/01_dependency/ that looks like:

'use strict'

const VError = require('verror') 
const foo = require('foo')

try {
  foo()
} catch (error) {
  console.log(error instanceof VError)
}

const error = VError('not foo')
console.log(error instanceof VError)

This example will output the following:

$ node 00_main/index.js
false
true

Notice that both the "main" application and the dependency rely on the exact same version of verror. In order for instanceof to work in this case, the 01_dependency would need to export its instance of VError so that 00_main could use it.

If verror were to implement [Symbol.toStringTag], the proposal in this PR would fix the issue.

example.zip

fox1t · 2022-11-03T08:29:37Z

My objection is on how this is supposed to be used. I would instanceof individual errors, but not a blanket check against all the errors this module can produce.

If somebody wants all Fastify core errors, they can do err.code?.indexOf('FST_') === 0.

I agree with this. This proposal is also about knowing if the error is a fastifyError, regardless if Fastify, a plugin, or user code threw it. You are right, though, when you say that it is achievable checking for code and statusCode. I was thinking about providing a utility that can lower the barrier for the developers. I saw that error handling is usually one of the worst parts of the applications, and one of the barriers is handling application errors vs. unexpected errors. Boom has isBoom to allow the users check the error inside errorHandlers and act accordingly.

Hope this makes sense.

Uzlopak · 2022-11-03T09:14:49Z

Also dont forget jest. It messes also instanceof Error up.

Maybe we should find a solution based on Symbol.hasInstance?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/hasInstance

mcollina · 2022-11-04T22:01:30Z

Here is my taken on how I would implement this for Fastify:

if (err.code.indexOf('FST_') === 0) {
  // All Fastify errors go there.
}

jsumners · 2022-11-05T02:22:42Z

How would that solve the generic usage case of this module? I think the module name is confusing the goal. As I understand the desire, it is to determine if an error instance has been created by this module. Not necessarily that an error instance is one from the Fastify framework.

mcollina · 2022-11-05T06:59:10Z

I think the name of the module is incorrect in that sense as it has nothing to do with Fastify, but rather it's something we maintain.
I don't see the reason to test if an error has been generated by this module. Why would you do it?
I can see the reason to check if an error has code and statusCode properties set to meaningful values.

fox1t · 2022-11-05T10:30:59Z

@mcollina,, checking if an error is a fastify error could allow the users to know if the error is an application logic error (fastifyError) or any other unhandled error. This information will enable the users to act accordingly: send a report to external tools, set proper code for the response, etc. Basically, the ordinary operations that we all usually do inside error handlers.

Uzlopak · 2022-11-05T11:25:37Z

@fox1t
Repeating the same argument over and over does not make it more convincible.

mcollina · 2022-11-05T12:14:02Z

@fox1t the solution described in #86 (comment) is perfect for that use case as it uses error codes.

fox1t · 2022-11-05T13:57:20Z

@fox1t
Repeating the same argument over and over does not make it more convincible.

Pardon me? I was making the same point @jsumners made since I had the same impression that not everyone was on the same page. I don't have to convince anyone. Since I proposed this in the first place I tried to make sure that everyone understands what I am proposing.
@mcollina I agree with you. Here we are discussing if shipping a helper function could help the newcomers understand how to use this library as a generic error library instead of giving the impression it can be used only for Fastift core or Fastify plugins ecosystem. Since the error format is the same as I used in Interference and since we discussed the error format together a while ago, I think that this library can provide value outside of the Fastify ecosystem.

xtx1130

I think is a good case in https://github.com/hapijs/boom/blob/master/lib/index.js#L92-L108. Both using Symbol.hasInstance and special parameters to judge an instance

xtx1130 · 2022-11-08T01:18:25Z

README.md

@@ -37,6 +39,26 @@ const CustomError = createError('ERROR_CODE', 'Hello %s')
 console.log(new CustomError('world')) // error.message => 'Hello world'
 ```

+#### isFastifyError
+
+The module exports a isFastifyError-function that you can use to check if a value is of type FastifyError, it takes one parameters:


Suggested change

The module exports a isFastifyError-function that you can use to check if a value is of type FastifyError, it takes one parameters:

The module exports a isFastifyError-function that you can use to check if a value is of type FastifyError, it takes one parameter:

Uzlopak · 2023-09-01T14:55:27Z

Closing due to inactivity (lol)

add isFastifyError

af16873

Uzlopak requested a review from jsumners November 2, 2022 12:09

jsumners requested changes Nov 2, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

add documentation, rename parameter of isFastifyError from obj to val…

ad381dc

…ue, fix typings

modify toStringTag to return FastifyError instead of Error

f758693

Uzlopak commented Nov 2, 2022

View reviewed changes

Uzlopak requested review from Eomm and mcollina November 2, 2022 12:22

mcollina requested changes Nov 2, 2022

View reviewed changes

Eomm reviewed Nov 2, 2022

View reviewed changes

xtx1130 suggested changes Nov 8, 2022

View reviewed changes

This was referenced Dec 16, 2022

Fix minification issue / make implementation more solid ldapjs/node-ldapjs#824

Closed

Fix minification issue with FilterString ldapjs/node-ldapjs#825

Closed

jsumners mentioned this pull request Aug 28, 2023

Incorrect usage of the Error class fastify/fastify#4998

Closed

2 tasks

Uzlopak closed this Sep 1, 2023

jsumners mentioned this pull request Jun 3, 2024

How should a Pino instance be identified in runtime? pinojs/pino#1984

Open

jsumners mentioned this pull request Jun 19, 2024

Please exports your errors type fastify/fastify-bearer-auth#182

Open

2 tasks

	The module exports a isFastifyError-function that you can use to check if a value is of type FastifyError, it takes one parameters:
	The module exports a isFastifyError-function that you can use to check if a value is of type FastifyError, it takes one parameter:

add isFastifyError #86

add isFastifyError #86

Conversation

Uzlopak commented Nov 2, 2022

Checklist

Uzlopak commented Nov 2, 2022

Uzlopak commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak commented Nov 2, 2022 • edited Loading

mcollina commented Nov 2, 2022

jsumners commented Nov 2, 2022

mcollina commented Nov 2, 2022

Uzlopak commented Nov 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fox1t commented Nov 2, 2022

Uzlopak commented Nov 2, 2022

mcollina commented Nov 2, 2022

jsumners commented Nov 2, 2022

mcollina commented Nov 2, 2022

jsumners commented Nov 2, 2022

fox1t commented Nov 3, 2022

Uzlopak commented Nov 3, 2022

mcollina commented Nov 4, 2022

jsumners commented Nov 5, 2022

mcollina commented Nov 5, 2022

fox1t commented Nov 5, 2022

Uzlopak commented Nov 5, 2022

mcollina commented Nov 5, 2022

fox1t commented Nov 5, 2022

xtx1130 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak commented Sep 1, 2023

Uzlopak commented Nov 2, 2022 •

edited

Loading