-
Notifications
You must be signed in to change notification settings - Fork 29
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
chore(open-payments): adding debug logs for client initialization & requests #485
Conversation
🦋 Changeset detectedLatest commit: 2a9b27f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -56,7 +56,7 @@ const incomingPayment = await client.walletAddress.get({ | |||
| ------------------- | --------------------------------------------------------------------------------------------------------------- | | |||
| `requestTimeoutMs` | (optional) The timeout in ms for each request by the client. Defaults to 5000. | | |||
| `logger` | (optional) The custom logger to provide for the client. Defaults to pino logger. | | |||
| `logLevel` | (optional) The log level for the client. Defaults to `info`. | | |||
| `logLevel` | (optional) The log level for the client. Defaults to `silent`. | |
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.
I think external libraries like ours should only log if explicitly specified
✅ Deploy Preview for openpayments-preview canceled.
|
throw new OpenPaymentsClientError(errorMessage, { | ||
description | ||
}) |
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.
Might be worthwhile to pass Error.cause
for better debugging sometime.
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.
Yes, that would be a nice improvement to the errors. I think this is something to tackle when updating Typescript in the repo, as we currently don't have the updated types for Error
class that includes cause
. We can do it then such that we do it properly instead of using ts-ignore
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.
Aside: On updating TypeScript, I was updating @apidevtools/json-schema-ref-parser
today (due to some node-fetch error it apparently causes on Windows), but then it required updating pretty much all dependencies, from openapi-typescript to TypeScript to eslint 😄.
Should do that sometime - nearly all dependencies pretty out of date
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.
Should do that sometime - nearly all dependencies pretty out of date
Yes, maybe in a next sprint I can go through 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.
PR looks fine to me, but I haven't tested it locally to feel it.
Actions failing, cause: |
fd29649
to
ed5ee5a
Compare
- uses: pnpm/action-setup@v2 | ||
with: | ||
version: 8 | ||
- uses: pnpm/action-setup@v4 |
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.
with: | ||
version: 8 |
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.
Otherwise:
Error: Multiple versions of pnpm specified:
- version 8 in the GitHub Action config with the key "version"
- version pnpm@8.[15](https://github.com/interledger/open-payments/actions/runs/9794096762/job/27043408325?pr=485#step:3:17).6 in the package.json with the key "packageManager"
Remove one of these versions to avoid version mismatch errors like ERR_PNPM_BAD_PM_VERSION
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.
I'm inclined to do the pnpm update as a separate PR, but ok anyway.
ed5ee5a
to
2a9b27f
Compare
Changes proposed in this pull request
silent
Example logs when GETting a wallet address, and making an incorrect grant request:
Context
OpenPaymentsClientError
#482