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

add TypeScript type definitions. #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

endel
Copy link

@endel endel commented Sep 12, 2018

Hey there,

I've added TypeScript type definitions for the verifyPayment method, which includes:

  • Platform type with supported platforms
  • Payment interface
  • Response interface

Cheers!

@justinpage
Copy link
Contributor

I'm kinda in the middle on this PR. Our main service is written in TypeScript so I see the advantage of having this available (e.g. avoiding any).

At the same time, I wonder if this is a community standard, where packages are supporting TypeScript definitions --especially when it's not used in src. One example I found: https://github.com/axios/axios

What do you think? @ronkorving

Happy to test as well.

@ronkorving
Copy link
Collaborator

I like this.
I like TypeScript, but am not a big user of it. One of the things I don't really like is when you need to depend on https://github.com/DefinitelyTyped/DefinitelyTyped to get these definitions into your project (since they're maintained independently from the source project). I think if we're going to do this, the maintenance burden (keeping it in-sync with the actual JS API) should lie here, in this project. So I like it living here.

However... What are the best practices for maintaining this? How can people who don't use TypeScript still safely make contributions without breaking the type definitions file all the time?

@endel
Copy link
Author

endel commented Sep 24, 2018

@ronkorving I can add a test case for the type definitions, which is a simple check if end-user calls match the described API, and compiling it without generating an output (tsc test.ts --noEmit):

// test.ts
import * as iap from "../";

iap.verifyPayment("google", {
    receipt: {},
    productId: "product",
    packageName: "com.example",
    secret: "123",
}, (err, response) => {
});

I just noticed that this library doesn't have unit testing for JavaScript, which can also be difficult for safely making contributions 😱

@justinpage
Copy link
Contributor

justinpage commented Sep 24, 2018

Yeah some tests would go a long way for this package. Probably another issue or pull request needed for that effort. I'm curious if we would just adopt some unit tests with mocked data or setup a test sandbox that @ronkorving would allow us to run against.

in-app-purchase package does this but I think its for the sake of end-to-end testing. One downside to this is the burden falls on the owner to maintain --I'm not too crazy about this option.

I'm in favor of mock data with unit test.

@ronkorving
Copy link
Collaborator

I couldn't agree more. I would love a test suite very much. The nature of this packages seems to make it so that it's hard to have a dedicated integration test, so mocking may just have to do the trick. I'm all for it 👍

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