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.

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

Implement FastifyErrorCode enum #5437

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bradennapier
Copy link

@bradennapier bradennapier commented May 1, 2024

Checklist

Summary

I will be glad to complete the missing items should consensus show positive attitude towards this PR.

Currently its quite a pain to work with FastifyError due to the fact there is no programmatic way to clearly parse an error for a specific type.

You either have to:

  1. Use instanceof checks against each check individually in an errorHandler
  2. Use some form of hackery such as function.name.toString()
  3. Use something like error.code.startsWith('FST') to identify fastify errors against internal errors

While I agree that this should also include changes to @fastify/errors to include something like error.isFastifyError checks in typescript, the TypeScript side of this is equally as painful:

  • As seen in this discussion it is quite painful to deal with fastify error codes in any way that works well in typescript or which can be relied upon against upstream changes
  • Internal packages are relying on a "hack" to FastifyError checks just to make it work such as:
    • error instanceof Error && 'code' in error && error.code === 'FST_JWT_BAD_REQUEST' (not type safe if that code were to change , even if that is very unlikely)
    • error instanceof Error && 'code' in error && error.code.startsWith('FST')

There are quite a few patterns that could be used to implement more convenient handling here, so this PR serves as a starting point to discuss potential ds


Changes Proposed

  • A FastifyErrorCode enum which is an object (typescript enum pattern) of { [code]: code } allowing for runtime checks against possible fastify error codes in their error handlers
  • A FastifyErrorCode type which is identical to the object allowing easily checking in typescript code with strict typing
  • use the FastifyErrorCode enum to construct the codes object directly, making it the source of truth
  • No breaking changes and still re-uses the same error patterns for the most part

Another way of implementing this on the javascript side would be to take the current codes and construct the enum from that in that Object.entries(Object.keys(codes).map(code => [code, code])) -- although i felt just statically defining it was better choice

This allows for easily handling errors from fastify in that we can now use checks (note that ideally FastifyError would have code set to FastifyErrorCode , but i didnt do that as i could see how that might be more controversial due to potential user modifications

function handleFastifyError(error: FastifyError) {
  switch (error.code) {
    case FastifyErrorCode.FST_ERR_ROUTE_METHOD_INVALID:
    case FastifyErrorCode.FST_ERR_ROUTE_METHOD_NOT_SUPPORTED: {
      break;
    }
    case FastifyErrorCode.FST_ERR_CTP_INVALID_TYPE:
    case FastifyErrorCode.FST_ERR_CTP_EMPTY_TYPE:
    case FastifyErrorCode.FST_ERR_MISSING_CONTENTTYPE_SERIALIZATION_FN:
    case FastifyErrorCode.FST_ERR_CTP_INVALID_MEDIA_TYPE: {
      break;
    }
    case FastifyErrorCode.FST_ERR_CTP_BODY_TOO_LARGE: {
      break;
    }
    case FastifyErrorCode.FST_ERR_NOT_FOUND: {
      return 
      break;
    }

    default:
      break;
  }
}

@fastify/error

This specific PR should also eventually include more changes to make error handling easier against fastify internal errors:

  • Include error.isFastifyError or similar checks which will allow narrowing against Error | FastifyError without instanceof checks or the need to use a base error which the maintainer is against (instanceof FastifyError)
  • Potentially include errorCodes.FST_JWT_BAD_REQUEST.code as a static property of the constructed error as another way of switching against what error occurred.

Export a `FastifyErrorCode` object in errors which allows better handling of error codes in the case a Fastify error code occurs

Signed-off-by: Braden Napier <bradynapier+github@gmail.com>
@bradennapier
Copy link
Author

bradennapier commented May 1, 2024

Note that I am personally more in favor of the type portion of FastifyErrorCode being a union in that

export const FastifyErrorCode = { [code]: code } as const
export type  FastifyErrorCode = typeof FastifyErrorCode[keyof typeof FastifyErrorCode]

as this allows using it like a enum :

function handleFastifyErrorCode(code: FastifyErrorCode) {
  switch(code) {
      case FastifyErrorCode.FST_ERR_CTP_EMPTY_TYPE: {
          ...
          break;
      }
  }
 }

However, I dont see that pattern used much outside my own codebases so I went with the more common pattern

Copy link
Member

Choose a reason for hiding this comment

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

The changes in this file will be very annoying to maintain. I would much rather see a method for inspecting error objects as outlined in fastify/fastify-error#17 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

It appears that way, yet it doesn't change much. You still need to define the string manually twice then again in the types.

It's not ideal, no. However even with the IsFastifyError implemented there is still no way to then easily narrow the specific errors you want to single out for handling.

The other option which requires almost zero cost to maintain is to generate the enumeration from the code's object directly - but the code's object still requires similar maintenance to this PR

In our case there are some cases we want exposed to users with our error messaging while others are internal errors we only wish to log internally while returning an internal server error to the user.

In my opinion both this and the isFastify concepts are required and complement each other.

Doing 20+ instanceof checks over a switch that can return never case to warn me if new errors are added in future that I am not handling is ideal.

Copy link
Member

Choose a reason for hiding this comment

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

The current code just double the works to do when adding a new error.
I am not oppose to update the types but in Javascript side, does it necessary for the change?
You can use Object.keys to generate the enum if you need.

@mcollina
Copy link
Member

mcollina commented May 2, 2024

Could clarify what problem are you trying to solve here? Maybe with an example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants