fix: instanceof MedusaError does not work
#9094
Conversation
|
|
@McTom234 is attempting to deploy a commit to the medusajs Team on Vercel. A member of the Team first needs to authorize it. |
McTom234
changed the title
instanceof MedusaError does not workinstanceof MedusaError does not work
as requested medusajs#9094 (comment)
|
@riqwan Oh, I see, at least one test fails, maybe some more. The default name of the error will change. As you can test locally, the As you test for the name of the error, do you rely on that somewhere in the code or should I just update the integration tests? |
I think this is a safe change, you just need to update the test. We handled this issue differently ourselves - ref. I don't know if that will be translatable here. Will loop in @adrien2p for a second opinion. |
| @@ -46,6 +46,8 @@ export class MedusaError extends Error { | |||
| constructor(type: string, message: string, code?: string, ...params: any) { | |||
| super(...params) | |||
|
|
|||
| this.name = this.constructor.name | |||
There was a problem hiding this comment.
I would suggest to do the following to follow our practices and be consistent with the rest of the code base
export class MedusaError extends Error {
// we do not use Symbol because it is not serializable
__isMedusaError = true
public type: string
public message: string
public code?: string
public date: Date
public static Types = MedusaErrorTypes
public static Codes = MedusaErrorCodes
static isMedusaError(error: any): error is MedusaError {
return !!error.__isMedusaError
}and then when checking somewhere you can do MedusaError.isMedusaError(err)
wdyt?
There was a problem hiding this comment.
You have your point 👍
In that case, I would add a hint in the docs as well?
There was a problem hiding this comment.
Sure, open that as a different PR as it requires different reviewers.
|
@riqwan I applied the changes. Could you run the pipelines again? |
|
Seems good to go @riqwan 😄 |
|
@McTom234 legend! Thanks for the contribution! |
The same as #9094, but for v1. Our project won't be moving to v2 soon probably, and we want the feature of course 😄 Co-authored-by: Riqwan Thamir <5105988+riqwan@users.noreply.github.com>
Closes #9088
As stated in the issue and this block article, adding one line of code, fixes the
instanceofmethod.What
Set the name of the
MedusaErrortype.Why
Developers can differ between
MedusaErrors and other errors easily.I already have an idea/suggestion to improve the default error handler with this new possibility (see below).
Testing
Somewhere in your code, throw a
MedusaErrorand a genericError. Try this with and without this fix. For both, get theinstanceof Errorandinstanceof MedusaError.Before the fix, both return true for Error. After, the medusa error will additionally return true for the
MedusaError.Because of that, no breaking changes were made.
Further possibilities
The current behavior of the
errorHandlerfor generic/non-medusa-errors is to return a500with messageAn unknown error occurred.This is cool in most cases. Some developers might prefer to get the actual error message returned, if it is not a
MedusaError. Theinstanceofcan be used in that case. This configuration could be added via the medusa config or via the parameters for the defaulterrorHandlerfunction.This shall not be part of this PR and just an insight on why this might be useful.