Skip to content
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

fixing types and other #114

Closed
wants to merge 2 commits into from
Closed

Conversation

sambegin
Copy link

@sambegin sambegin commented Feb 3, 2022

  • Adding dist and .idea to .gitignore (helps when you're trying to improve this lib)
  • Changing string to number for payment (would probably need to be changed for many more) since it fits with your documentation: https://developer.gocardless.com/api-reference/#payments-create-a-payment Everything seems to be numbered, not string, why keeping string in types :) ?
  • Adding types to webhook (you already had the types defined, but the function did not provide them. We could not use it in typescript projects)
  • Changing the webhook export (since again, was not able to import properly in typescript). This is a duplicate from: Update webhook export #111

@@ -1,7 +1,6 @@
{
"compilerOptions": {
"module": "commonjs",
"moduleResolution": "Node",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also forgot: this was duplicated

@dreinon
Copy link

dreinon commented Mar 14, 2022

@markrofail-gc Sorry to tag you, just doing it in case you missed this, since these changes are important to merge 🙂
Thanks!

parse,
InvalidSignatureError,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Why is this error not being retured anymore?
Thanks 🙂

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't know why GitHub didn't notify me about your reply 🤦‍♂️

That's a good question (I would have probably a better response three weeks ago), but from what I see I probably thought that it was a mistake (because we would want to export the verifySignature instead of InvalidSignatureError). But if you want to check the error instance somewhere I guess you need that.

I can re-add it back

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dreinon Done,

Sorry for the late response, I'll double check my github settings 🤦‍♂️

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No worries! We can't do much until the gocardless team replies back haha

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn I thought you were on the gocardless team 😭

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not :(

@szastupov szastupov mentioned this pull request May 3, 2022
@szastupov
Copy link
Contributor

Thanks for the PR and sorry for the lack of activity. As the PR changes the generated code, we cannot accept it, but the issues have been fixed in #122

@szastupov szastupov closed this May 3, 2022
@sambegin
Copy link
Author

@szastupov Thanks for the update.

The webhook signature has been fixed with your PR, however, all amount fields still don't have the good type. They are defined as string but number is used everywhere in your documentation (and it's also number we use to call this lib).

Any chance you can update this ?

@szastupov
Copy link
Contributor

Hi @sambegin! Just want to let you know that we've added fixing union types like amount to our backlog 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants