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

TS Fixes: arrays of subobjects, emptyStringable fields #802

Merged
merged 3 commits into from
Feb 14, 2020

Conversation

richardm-stripe
Copy link
Contributor

@richardm-stripe richardm-stripe commented Feb 12, 2020

r? @ob-stripe
cc @jlomas-stripe @stripe/api-libraries

  1. More consistently encode "emptyStringable" types (described in the openapi spec as anyOf(X, enum([''])) as X | null instead of X | ''.
  2. Fixes issue Update 'custom_fields' type to match documentation #766
  3. A little bit of churn re: ordering of things because of codegen changes.
    Fixes Incorrect typings custom_fields when  #794

@ob-stripe
Copy link
Contributor

Many fields that were typed as a union with null are changed to a union with '' to better reflect the input expected by the API.

This is correct at the API level, but in many of our client libraries we prevent users from passing '' and require them to pass null, which we then encode as an empty string in the request. I don't know offhand if that's the case in stripe-node, could you double-check?

@richardm-stripe
Copy link
Contributor Author

richardm-stripe commented Feb 12, 2020

@ob-stripe - Good point. We don't actively prevent setting an empty string in node.js, but ultimately API Requests are urlencoded null and '' are urlencoded identically over the wire. So we should decide solely on the basis of what type seems more natural to the user.

I think that's | null rather than | '' in most cases, so I've modified the codegen accordingly, but this has resulted in a new set of changes because the existing code generation did not consistently produce | null either (it did | null for anyOf(class, enum([''])), but did | '' in cases like

  • anyOf(array<...>, enum(['']))
  • anyOf(primitive, enum(['']))
  • anyOf(sharedClass, enum([''])).

@richardm-stripe
Copy link
Contributor Author

The other possibility would be defining
type EmptyParam = null | '' which would be backwards compatible but I think it is better to standardize on null.

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.

Agree that null is a better choice than ''.

LGTM!

@richardm-stripe richardm-stripe merged commit a40ff56 into master Feb 14, 2020
@richardm-stripe richardm-stripe deleted the richardm-autogen-fixes branch February 14, 2020 18:20
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.

Incorrect typings custom_fields when
3 participants