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
feat(client): add client version to errors #3757
Conversation
async $connect() { | ||
return this._engine.start() | ||
try { | ||
return this._engine.start() | ||
} catch (e) { | ||
e.clientVersion = this._clientVersion | ||
throw e | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more try-catches in the top-level methods, such as connect
, disconnect
, executeRaw
, queryRaw
& transaction
. While there's already some errors caught at the PrismaClientFetcher.request
level, a lot of stuff happening before that which could go wrong.
If an error is caught/rethrown at the PrismaClientFetcher.request
level, top-level methods should also catch it and re-throw the error, without touching it this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good :)
if (internal.hooks) { | ||
this._hooks = internal.hooks | ||
} | ||
this._clientVersion = config.clientVersion ?? clientVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client version is passed from the generated client in the config object. I'm not sure why the field is optional though so I'm fallbacking to the local package.json version in case it's not there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could probably make it required, this is rather an old artifact, as in some tests we sometimes didn't have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is really great! It would be awesome if you could add a simple integration test to make sure, that the version is indeed available on an error.
You can read more here https://github.com/prisma/prisma/blob/master/CONTRIBUTING.md#creating-a-new-integration-test how to create an integration test
Looks like you need to merge master in to update the snapshots |
Closes #3662