Wizcorp / node-iap

In-app purchase validation for Apple, Google, Amazon, Roku
262 stars 92 forks source link

add TypeScript type definitions. #54

Open endel opened 6 years ago

endel commented 6 years ago

Hey there,

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

Cheers!

justinpage commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 👍