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: add 'lastResponse' to return types #990

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Aug 20, 2020

r? @ctrudeau-stripe @jlomas-stripe
cc @remi-stripe @stripe/api-libraries

Fixes #983

Approach:

In codegen, all the API responses that correspond to single resources now return e.g. Stripe.Response<Customer> instead of Customer. What is Stripe.Response?

export type Response<T> = T & {
  headers: {[key: string]: string};
  lastResponse: {
    requestId: string;
    statusCode: number;
    apiVersion?: string;
    idempotencyKey?: string;
    stripeAccount?: string;
  };
};

As far as I can reason, intersection types are subtypes of their constituents, so this should be non-breaking and safe.

List responses are a little trickier. I didn't change anything in the codegen, because each method definition (e.g. stripe.accounts.listExternalAccounts has a type like

/**
 * List external accounts for an account.
 */
listExternalAccounts(
  id: string,
  params?: ExternalAccountListParams,
  options?: RequestOptions
): ApiListPromise<Stripe.BankAccount | Stripe.Card>;
listExternalAccounts(
  id: string,
  options?: RequestOptions
): ApiListPromise<Stripe.BankAccount | Stripe.Card>;

so instead I redefined ApiListPromise to extends Promise<Stripe.Response<ApiList<T>>> instead of extends Promise<ApiList<T>>

I was also a bit concerned about the "intersection type of union types". In theory, Response<BankAccount | Card> should be the same as Response<BankAccount> | Response<Card>, and so when a user does a refinement (i.e. they do if (res.object === 'card') {...}), inside their if statement, resshould be aResponse`. I checked it and in practice this seems to work.
Testing strategy:

Added property accesses to the typescriptTest.js for lists and a non-list response.

@richardm-stripe richardm-stripe force-pushed the richardm-add-lastResponse branch 6 times, most recently from ab1e35e to df22a30 Compare August 20, 2020 19:22
@richardm-stripe richardm-stripe changed the title [WIP] Typescript: add 'lastResponse' to return types Typescript: add 'lastResponse' to return types Aug 20, 2020
Copy link
Contributor

@ctrudeau-stripe ctrudeau-stripe left a comment

Choose a reason for hiding this comment

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

LGTM

@richardm-stripe richardm-stripe force-pushed the richardm-add-lastResponse branch from 2a064e7 to c6a9d5b Compare August 27, 2020 14:46
@richardm-stripe richardm-stripe merged commit b52792a into master Aug 27, 2020
@richardm-stripe richardm-stripe deleted the richardm-add-lastResponse branch August 27, 2020 14:47
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.

Typescript access to lastResponse headers
3 participants