Improve UX of TypeDBClientError #187
Conversation
| get errorMessage(): ErrorMessage { | ||
| return this._errorMessage; | ||
| get messageTemplate(): ErrorMessage { | ||
| return this._messageTemplate; |
There was a problem hiding this comment.
The trouble with the name errorMessage is that TypeDBClientError extends Error which has a property named message.
message and errorMessage are synonymous in this context; messageTemplate, less so.
| else if ("code" in error) { | ||
| if ([Status.UNAVAILABLE, Status.UNKNOWN, Status.CANCELLED].includes(error.code) || error.message.includes("Received RST_STREAM")) { | ||
| this._errorMessage = UNABLE_TO_CONNECT; | ||
| super(UNABLE_TO_CONNECT.message()); |
There was a problem hiding this comment.
So previously we had this confusing situation where, if you had a connection failure, error.errorMessage was "Failed to connect to TypeDB server" and error.message was "14 UNAVAILABLE: No connection established".
And if you had a query error, error.errorMessage was null and error.message was "[QRY04] Query Error: ..."
This resulted in awful application code where we would check the errorMessage, and if there wasn't one, check the message, and so on.
Now, message is a built-in property of Error and it is strongly recommended to not set the property manually, because it is used by various other properties such as toString and stack. Instead, the correct JS way is to call super, passing in the message as a parameter.
But, super must be the first assignment statement in a constructor.
So we've ended up with this construction: we do all the checks, and then call super with the appropriate message and then set _messageTemplate if applicable.
As a result, the user now only ever needs to worry about message, except in special cases like detecting connection failures.
| } | ||
| super(CLUSTER_REPLICA_NOT_PRIMARY.message()); | ||
| this._messageTemplate = CLUSTER_REPLICA_NOT_PRIMARY; | ||
| } else if (error.code === Status.INTERNAL) super(error.details) |
There was a problem hiding this comment.
And here we need error.details as opposed to error.toString, because otherwise we get: "Error: 13 INTERNAL: [CLI01]: Client Error: The transaction has been closed and no further operation is allowed", which is not a very nice message. details is a property of gRPC's ServiceError.
flyingsilverfin
left a comment
flyingsilverfin
left a comment
There was a problem hiding this comment.
Cool! Do you think the title reflects the most important change? Is it the printouts the user of the client sees, or the API/UX of using the TypeDBClientError class?
## What is the goal of this PR? There was an error in test-deployment-pip caused by old syntax, which we fixed. ## What are the changes implemented in this PR? Fix error in test-deployment-pip
2 participants
What is the goal of this PR?
Previously, the
TypeDBClientErrorconstructor was a bit of a mess, meaning that under certain scenarios, the user was interested inerror.message; in other scenarios,error.errorMessage, and this was pretty ridiculous because the two names are basically synonymous.So we renamed
errorMessagetomessageTemplate, and ensured that the content oferror.messageis always a human-readable string.What are the changes implemented in this PR?
TypeDBClientError.errorMessagetomessageTemplateto reduce likelihood of confusing it withmessageerror.messageis always a human-readable string