-
Notifications
You must be signed in to change notification settings - Fork 35
feat(checkout): Add response validation schema and report if validation error #2683
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
base: main
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 77e954f
☁️ Nx Cloud last updated this comment at |
@@ -1,6 +1,6 @@ | |||
{ | |||
"root": true, | |||
"extends": [ |
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.
there was a conflicting jsx-ally rule being loaded from both these inherits so had to remove one. I tried adding the one from the root and remove next/
but it resulted in a lot of linting errors that need to be addressed on its own. Currently no lint is really applied as the package.json is linting only *.ts
files inside src/
which is ignored by the ignorePatterns from the root eslint rule since it is missing root: true
which is added above.
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.
Had some comments... did you test it with the widget sample app? Or by calling the real api?
if (params.nextPage) url += `&${new URLSearchParams(params.nextPage as Record<string, string>)}`; | ||
|
||
// Use the generic httpGet method with schema validation | ||
const response = await this.httpGet<BlockscoutERC20Response>(url, BlockscoutERC20ResponseSchema); |
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.
Do you need to pass the type parameter given you're passing the schema? The return type should be inferrable from that
|
||
export const BlockscoutTokenPaginationSchema = z.record( | ||
z.string(), | ||
z.union([z.string(), z.number(), z.null()]), |
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.
why like this? and not declaring the specific keys?
// Only validate response structure for successful responses (2xx) | ||
if (response.status >= 200 && response.status < 300) { | ||
try { | ||
const validatedData = schema.parse(response.data); |
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.
slightly preferable would be to use safeParse to handle the error case- not a blocker though
} | ||
|
||
// For non-2xx responses, return data without validation | ||
return Promise.resolve(response.data); |
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.
Doesn't seem quite right - if the response is 400, the return type should probably be unknown
Or it should reject... what does this.httpClient.get
currently do if it gets a >= 300 response?
Further, use the error reporting to alert us of the change in fields so we can proactively figure it out.