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

TypeScript definitions #736

Merged
merged 95 commits into from
Dec 24, 2019
Merged

TypeScript definitions #736

merged 95 commits into from
Dec 24, 2019

Conversation

rattrayalex-stripe
Copy link
Contributor

@rattrayalex-stripe rattrayalex-stripe commented Dec 5, 2019

This adds TypeScript definitions for stripe-node, reflecting the latest API version.

To try it out, install with:

yarn add https://github.com/stripe/stripe-node#typescript
# or 
npm install --save https://github.com/stripe/stripe-node/tarball/typescript

Docs are here.

Feedback from the community would be much appreciated on this! Email me at [email protected] or leave comments here.

Fixes #296

@bruun
Copy link

bruun commented Dec 6, 2019

First of all, this is awesome 🙌

I've given it a quick go in our codebase, and with strict null checks enabled we immediately got a lot of type errors due to the id properties on retrieved resources (PaymentIntent, Transfer, etc) being optional. Do ids in general need to be optional?

The same goes for certain properties on objects, such as amount and currency on a PaymentIntent. Can these actually be optional when they are required in the PaymentIntentCreateParams?

Other issues I came across in my brief testing this morning:

  • stripe.accounts.retrieve seems to be missing a signature for retrieving a Custom Account by id.
  • We're now getting an error when importing Stripe with import * as Stripe from 'stripe' (using module: commonjs in our .tsconfig):
    • This module can only be referenced with ECMAScript imports/exports by turning on the 'esModuleInterop' flag and referencing its default exports

EDIT: Found the part about importing using import Stripe from 'stripe' in the docs, but that causes a similar error:

image

@rattrayalex-stripe
Copy link
Contributor Author

Awesome, thanks @bruun , this is super helpful. Appreciate all the specifics very much, will look into all of those shortly.

Updated the docs to mention the esModuleInterop flag, which unfortunately you do need to use

@rattrayalex-stripe
Copy link
Contributor Author

Thanks so much again for the feedback @bruun – all your comments have been addressed. Care to give it another shot?

@bruun
Copy link

bruun commented Dec 12, 2019

No problem @rattrayalex-stripe :)

The change to optional fields helped a lot, thanks!

One last thing that I can think of at the moment, it would be great if definitions like this:

/**
* The business type. Can be `individual` or `company`.
*/
business_type: string | null;

Could be limited to the actual possible values

/**
* The business type. Can be `individual` or `company`.
*/
business_type: "individual" | "company" | null;

@rattrayalex-stripe
Copy link
Contributor Author

rattrayalex-stripe commented Dec 12, 2019

Terrific, thanks @bruun ! How much work was it to move your project over from DT? Did you need to change a lot of things or just install & it worked?

Yes, we have a project underway to find all of those strings and convert them over to enums!

Curious for your thoughts on our reliance on the tsconfig.json types field. It's a bit unorthodox but we want to make it clear that these types represent only a single API version (and give people
time to port over from DefinitelyTyped, if that's onerous).

@bruun
Copy link

bruun commented Dec 12, 2019

Moving over from DT was pretty painless, and mostly consisted of renaming Stripe.paymentIntents.IPaymentIntent -> Stripe.PaymentIntent etc. The new type definitions even revealed some bugs in our code :-)

The hard part, and something I haven't managed to solve yet due to time constraints, is making our codebase work with esModuleInterop: true. It introduces a ton of errors unrelated to Stripe we'll need to fix, and many runtime errors not caught by the TypeScript compiler. There might be a quick fix for this, and it certainly isn't the Stripe library's fault, but it does mean we can't switch from DT right away.

The reliance on the types field in tsconfig.json is indeed a bit unorthodox, but not a problem for me personally. There are probably mixed opinions here. However, I noticed one issue that might be related:

If we do not explicitly import Stripe from 'stripe' in a module, this code results in a TS error:

image

The above error disappears if we explicitly import Stripe in that module.
However, if we don't import Stripe and change the code to the following, the error disappears:

image

There might be something very off in our module resolution which might also explain the esModuleInterop problems, but I thought I'd mention it just in case.

@rattrayalex-stripe
Copy link
Contributor Author

rattrayalex-stripe commented Dec 13, 2019

Hi @bruun,

Thanks, that's great to hear! We love catching bugs.

We definitely don't want users to have to make runtime changes to adopt our types. I changed the export scheme so esModuleInterop isn't required anymore. Care to remove that and try again? Hopefully should be much smoother!

Regarding the errors, yes, you do need to import Stripe (or use the instantiated stripe client) to do an instanceof check, since they happen at runtime. In fact, instanceof Stripe.StripeInvalidRequestError should be a type error (hopefully it wasn't only because of the wonky setup).

With esModuleInterop gone, are you able to adopt it smoothly?

@bruun
Copy link

bruun commented Dec 15, 2019

@rattrayalex-stripe Removing the esModuleInterop requirement no longer breaks our other imports, but something is now wrong with the Stripe import and initialization.

The following does not give a type error, but fails at runtime:

// Typescript
import Stripe from 'stripe';
const stripe = new Stripe(API_KEY);``
 
// Compiles to
const Stripe = require("stripe");
const stripe = new Stripe(API_KEY);

// Fails at runtime with
const stripe = new Stripe(API_KEY);
               ^
TypeError: stripe_1.default is not a constructor

The following works at runtime, but yields a Typescript error (version 3.6.4):

// Typescript
import Stripe = require('stripe'); // (Same with keeping import * as Stripe from 'stripe')
const stripe = new Stripe(API_KEY);

// Compiles to
const stripe_1 = require("stripe");
const stripe = new stripe_1.default(API_KEY);

// Typescript error
Type 'typeof import("stripe")' has no construct signatures.ts(2351)
// Relevant properties from tsconfig
"target": "es2020",
"module": "commonjs",
"moduleResolution": "node",

Again, this could be a issue with our configuration or even my local setup, I'll try to look further into it on Monday.

@rattrayalex-stripe
Copy link
Contributor Author

Thanks again @bruun ! Sorry about that, I should have checked that kind of setup first. Thanks for sharing your config.

I've now made a few changes (see updated docs) – you can now import Stripe from 'stripe' and it'll work at runtime in TS, you no longer need to specify the "types" field in tsconfig.json, and you now ~must specify the api version when you instantiate Stripe:

import Stripe from 'stripe';
const stripe = new Stripe('sk_test_123', {apiVersion: '2019-12-03'});

Give that a try? Curious what you think.

@bruun
Copy link

bruun commented Dec 17, 2019

@rattrayalex-stripe Great, I can now successfully compile and run our code :-)

Personally I like the changes to the apiVersion, and having to explicitly ignore any errors. I think perhaps this part of the docs...

If you are on an older API Version (eg; 2019-10-17) and not able to upgrade, you may pass apiVersion: null to use your account's default API version.

... should include the more specific comment from types/lib.d.ts (emphasis mine):

If you wish to remain on your account's default API version, you may pass null or another version instead of the latest version, and add a @ts-ignore comment here and anywhere the types differ between API versions.

As we explicitly use an earlier API version than "2019-12-05", it wasn't immediately clear from the docs what the proper way of specifying this API version was. The second comment makes it very clear.


Coming back to the previously discussed instanceof issues, this still does not yield a type error until I explicitly import Stripe:

err instanceof Stripe.StripeInvalidRequestError

As you said previously, this fails at runtime because Stripe is not defined.

When not explicitly importing Stripe in a module, Stripe resolves to the namespace in lib.d.ts:

image

When explicitly importing Stripe, Stripe resolves to the class in index.d.ts:

image

Any ideas? I have made sure to remove Stripe from the tsconfig "types" array.

@rattrayalex-stripe
Copy link
Contributor Author

Fantastic!

Great suggestion; reworded. Let me know what you think.

I removed the direct export of the error class types; they're now only accessible from Stripe.errors. Does that clear things up?

@bruun
Copy link

bruun commented Dec 17, 2019

The docs are very clear now, thank you!

Removing the export on the class types didn't help, unfortunately. I have not worked much with namespaces and modules in Typescript so this is outside my comfort zone, but Typescript is apparently still able to find the StripeInvalidRequestError class type in the Stripe namespace, even when not explicitly exported.

@rattrayalex-stripe
Copy link
Contributor Author

Ah, darn. I'm not sure if there's anything we can do about that, then. Presumably it'd be rare for users to run into this, and it only happened to you because of the unfortunate import story from before?

@bruun
Copy link

bruun commented Dec 17, 2019

That sounds likely, yeah. Maybe someone will come along with a suggested fix once it's released and people start migrating from DT

@JetJet13
Copy link

JetJet13 commented Dec 20, 2019

Hey @rattrayalex-stripe,

I'm running into issues with the Stripe.invoices.listUpcomingLineItems method.

Edit: I forgot to mention that I'm using version 2019-12-03

I'm getting an error when I invoke listUpcomingLineItems as follows,

this.stripe.invoices.listUpcomingLineItems(customerId, {
      limit,
      starting_after: startingAfter,
    })
Error: Stripe: Unknown arguments (<customerId>,[object Object]). Did you mean to pass an options object? ... (on API request to GET `/invoices/upcoming/lines`)

Workaround

I have managed to find a workaround, which is to ignore the type definitions and insert the customerId inside the params object

    const method: any = this.stripe.invoices
    return method.listUpcomingLineItems({
      customer: customerId,
      limit,
      starting_after: startingAfter,
    })

Suggested Fix

Replace

listUpcomingLineItems(
      id: string,
      params?: InvoiceLineItemListUpcomingParams,
      options?: RequestOptions
): ApiListPromise<InvoiceLineItem>;


listUpcomingLineItems(
      id: string,
      options?: RequestOptions
    ): ApiListPromise<InvoiceLineItem>;

With

listUpcomingLineItems(
      params: InvoiceLineItemListUpcomingParams,
      options?: RequestOptions
): ApiListPromise<InvoiceLineItem>;


listUpcomingLineItems(
      params: InvoiceLineItemListUpcomingParams,
      options?: RequestOptions
    ): ApiListPromise<InvoiceLineItem>;

and update InvoiceLineItemListUpcomingParams to include customer

  interface InvoiceLineItemListUpcomingParams {
    /**
     * The identifier of the customer whose upcoming invoice line items you'd like to retrieve.
     */
    customer: string;
    .
    .
    .
}

@rattrayalex-stripe
Copy link
Contributor Author

Thanks for reporting @JetJet13 ! You're correct; I hope to fix this up today.
How has everything else been?

@JetJet13
Copy link

No real issues to report aside from the one mentioned. The transition from @types/stripe has been pretty good so far.

Thanks for picking this up!

@rattrayalex-stripe
Copy link
Contributor Author

@JetJet13 the issues with listUpcomingLineItems and InvoiceLineItemListUpcomingParams should now be fixed.

@bruun we're no longer polluting the global namespace, so the Stripe.StripeInvalidRequestError problem should now be fixed.

@rattrayalex-stripe rattrayalex-stripe changed the base branch from master to v8.0.0 December 24, 2019 02:09
@rattrayalex-stripe rattrayalex-stripe mentioned this pull request Dec 24, 2019
8 tasks
Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

Again, amazing job @rattrayalex-stripe and @jlomas-stripe!

I focused on the non-TS files and the manually written TS files (shared.d.ts, lib.d.ts, webhooks.d.ts, and OAuth.d.ts). Left one comment, but LGTM otherwise. I'd feel better if someone with more Node/TS experience reviewed too, but up to you.

*/
constructEvent(
payload: string,
header: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

payload and header can be either string or Buffers, cf.

payload = Buffer.isBuffer(payload) ? payload.toString('utf8') : payload;
header = Buffer.isBuffer(header) ? header.toString('utf8') : header;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks fixed. Note that this required adding @types/node as a dependency, meaning it could add a bit of bloat for non-TS users. Given this is a server-only package, I don't think that's the end of the world.

```ts
import Stripe from 'stripe';
const stripe = new Stripe('sk_test_...', {
apiVersion: '2019-12-03',
Copy link
Contributor

@richardm-stripe richardm-stripe Dec 24, 2019

Choose a reason for hiding this comment

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

Let's make sure our release script updates this automatically.


To install:
Types can change between API versions (eg; Stripe may have changed a field from a string to a hash),
Copy link
Contributor

Choose a reason for hiding this comment

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

e.g., I think is our style

const typescript = props.typescript || false;
if (typescript !== Stripe.USER_AGENT.typescript) {
// The mutation here is uncomfortable, but likely fastest;
// seralizing the user agent involves shelling out to the system,
Copy link
Contributor

Choose a reason for hiding this comment

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

serIalizing

@rattrayalex-stripe
Copy link
Contributor Author

Will correct typos in a follow-on, we'll likely have more work to do on README in any case.

@rattrayalex-stripe rattrayalex-stripe merged commit d96a741 into v8.0.0 Dec 24, 2019
@rattrayalex-stripe
Copy link
Contributor Author

Thanks to everyone in the community for the feedback!

You're welcome & encouraged to continue using this branch until v8.0.0 is released, likely in early-mid January, and please do continue to provide feedback, big & small!

rattrayalex-stripe added a commit that referenced this pull request Jan 10, 2020
* Drop support for Node 6

* TypeScript definitions [beta] (#736)

* First pass

* wip

* More changes

* Split resources by file

* Move to api version folder

* Remove tsconfig, add prettierignore

* Properly generate nested and no-method resources

* Document new TypeScript usage in README

* Add a basic test for typescript correctness

* Better formatting for export of Stripe

* Break out subtypes

* Separate out Address

* Pull empty strings out of enums

* Use all named subresources from openapi

* Rough draft of using shared subresources

* Revert "Rough draft of using shared subresources"

This reverts commit eb9a4d8.

* Fix some bugs I think

* Okay actually go back to normal

* Actually use all shared subresources from openapi

* Revert "Actually use all shared subresources from openapi"

This reverts commit 52136ae.

* Add webhooks.d.ts

* Add and test ApiListPromise for async iteration of lists

* Add better types for ApiList and HeaderOptions

* Rename HeaderOptions to RequestOptions

* Add docs and tests for expanding objects

* Add in reference to webhooks.d.ts

* Update snapshots to reflect reordering

* Add multiple dispatch for optional params

* Add TS tests for multiple dispatch, config, etc

* Move types for setAppInfo

* Add errors

* Fix some formatting

* Remove (Customer) Source and ExternalAccount

* Move metadata back up, add deleted?: void, add eg DeletedCustomer to the union for expandable deletable objects

* Reorder nested methods, rename nested & delete param types, move nested model & param types into their own files

* Remove docstrings from param types since they exist on method definitions

* Reorder required params to be first

* Minor method reorder

* Describe why you have to explicitly reference the types

* Move from vYYYY-MM-DD to YYYY-MM-DD, upgrade api version to 2019-12-03

* Update future version

* Fix param type name and mention esModuleInterop in docs

* Add docstring and test for ApiList

* Add docstrings for errors

* Fix accidentally optional fields

* Add missing values to RawErrorType

* Fix many non-optional fields

* Remove whitespace typo

* Add signature for retrieving a specific account

* Add types for OAuth

* Openapi updates

* Rename Webhooks in git

* Use export default instead of export = so that users dont need to do esModuleInterop

* Add a default export to the runtime

* Also export Stripe as a named export, and as a static property on itself

* Collapse types for setHost

* Require the user to specify the latest API version, use types by default.

* Fix a type error

* Reorganize test, use strictNullChecks etc

* Fix readme

* Unexport error classes to avoid instanceof confusion

* Clarify how to use old api versions with TS

* Fix typo params -> param

* Updates from openapi

* Mark the args to several setters as required

* More accurate handling of deleted resources and required/optional fields

* Erroneous handling of deleted

* Revert "Erroneous handling of deleted"

This reverts commit ad64e33.

* Add comment about deprecated total_count.

* Add shared types for Addresses and Metadata, and update openapi

* Use null intead of empty string

* Fix invoice upcoming lines

* Update openapi

* More empty-string to null translation

* Stop polluting the global namespace by scoping everything under module stripe

* Run eslint & prettier on TS files

* Continue to have poor types for HttpAgent, but with more claity as to why

* Clean up explicit any lint warnings

* Remove constants intended for internal use only

* Extract CustomerSource to a standalone type

* Remove unnecessary export

* Better types for account debit sources

* Use shared RangeQueryParam and PaginationParam

* Add expand param to SubscriptionDeleteParams

* Bring business_type back

* Add docstrings to config options

* Add mandatory `typescript: true` config prop

* Use await in TS readme

* Wrap await in async function

* Many readme updates

* Remove docs on non-public fields/params (we should revert this if it comes up a lot)

* Clarify async/await a bit

* TypeScriptify docstrings for Webhooks

* Add @types/node as a dependency, use it for typing Buffer and http.Agent

* Remove a TODO

Co-authored-by: jlomas-stripe <[email protected]>

* Small typo fixes (#743)

* Correct "eg;" to "e.g.,"

* Fix typo

* Remove deprecated resources and methods (#744)

* Remove stripe.usageRecords and stripe.usageRecordSummaries, add stripe.subscriptionItems.listUsageRecordSummaries

* Update openapi

* Remove Recipients

* Remove BitcoinReceivers

* Remove ThreeDSecure

* Fix SubscriptionSchedules.Plan.plan (#748)

It was incorrectly pointing back to SubscriptionSchedules.Plan instead of Stripe.Plan

* Openapi updates

* Use Stripe. prefix for all top-level resources (#751)

* Deprecate many library api's, unify others (#752)

* Deprecate snake_case options like stripe_account with warning messages

* Remove deprecated Error base export, extend method, and populate method

* Privatize several constants

* Add protocol to config object for local proxies etc

* Add `appInfo` config option

* Mark all setter methods as deprecated, emit warnings.

* Mark methods prefixed with an underscore as private

* Mark a few getter methods which are not prefixed with underscores as private

* Rename the request option stripeVersion to apiVersion to be consistent with the config option and our overall terminology

* Throw an error if a deprecated and non-deprecated version of the same request option is passed

* Accept an array of strings or buffers as the header argument for webhooks.constructEvent (#753)

* Stop importing errors as a single Error object, which shadowed the real Error object

* Accept an array of strings (or buffers) as the header in webhooks

* Actually just throw an error if an array is actually passed

* Improve markdown links in TS comments (#754)

Co-authored-by: jlomas-stripe <[email protected]>
Co-authored-by: Petja Touru <[email protected]>
@xenoterracide
Copy link

xenoterracide commented Jan 10, 2020

hey, we're using this, I know it's merged... but any chance more interfaces could be exported? specifically I wanted to make a type that configures our payment intents for both stripe and stripe terminal

const intent = await this.stripe.paymentIntents.create({
  ...payment.paymentMethod.options,
  amount: payment.getAmount(),
  currency: payment.getCurrency(),
  capture_method: 'manual',
});

and in one of my payment methods I have

export default class StripeCardPresent implements PaymentMethod {
  readonly options: { [key: string]: any };

  constructor() {
    this.options = {
      payment_method_types: ['card_present'],
    };
  }
}

what'd I'd really like to do is have PaymentIntentCreateParams exported so that I can make my type generic, and have it return this.

I can make a new ticket if y'all prefer.

@rattrayalex-stripe
Copy link
Contributor Author

@xenoterracide Yes, please open a new ticket!

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

Successfully merging this pull request may close these issues.

8 participants