Skip to content

fixing types and other - #114

Closed
sambegin wants to merge 2 commits into
gocardless:masterfrom
oxiotelecom:master
Closed

sambegin wants to merge 2 commits into
gocardless:masterfrom
oxiotelecom:master

Conversation

@sambegin

@sambegin sambegin commented Feb 3, 2022

Copy link
Copy Markdown
  • 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

Comment thread tsconfig.json
{
"compilerOptions": {
"module": "commonjs",
"moduleResolution": "Node",

Copy link
Copy Markdown
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

dreinon commented Mar 14, 2022

Copy link
Copy Markdown

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

Comment thread src/webhooks.ts
module.exports = {
export {
parse,
InvalidSignatureError,

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

3 participants