-
Notifications
You must be signed in to change notification settings - Fork 192
Fix: Node exception types #553
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
Conversation
|
Leaving this open until we address the consistency issue on web vs node |
|
When these changes merge ? |
|
@christyjacob4 @Meldiron can we push this forward? |
|
@lohanidamodar yes we can 👍 I believe the web sdk has been updated to include this as well |
templates/node/index.d.ts.twig
Outdated
| export class {{spec.title | caseUcfirst}}Exception extends Error { | ||
| public code: number; | ||
| public type: string; | ||
| public response: {{spec.title | caseUcfirst}}ExceptionResponse; | ||
|
|
||
| constructor(message: string, code?: number, type?: string, response?: {{spec.title | caseUcfirst}}ExceptionResponse); |
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.
@Meldiron can we make this consistent with the web sdk ?
https://github.com/appwrite/sdk-for-web/blob/master/src/client.ts#L84-L89
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.
Made web, node and Deno consistent.
Don't we need to do the same for all languages?
|
@Meldiron new version should have solved this right? Is this still relevant? |
|
@Meldiron closing this for now, please re-open if required. |
What does this PR do?
Type definitions in Node were not in sync with actual functionality regarding exceptions.
Test Plan
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
Yes 😊