Change how API versions are set

[sholladay]

Pinning all stripe requests to a specific API version is a good idea and recommended, yet the way this is currently done using the SDK is a bit strange and, I would argue, even a bit dangerous.

const Stripe = require('stripe');
const stripe = new Stripe('my-secret-key');
stripe.setApiVersion('2019-02-19');

The setApiVersion() method is strange because it is setting an option that really belongs in either the constructor or each API method call. As far as I am aware, it is the only option that uses a method to set it. Currently, you cannot do new Stripe({ apiVersion : '2019-02-19' }), which makes it inconsistent with how the secret key is set. You also cannot do stripe.customers.retrieve(customerId, { apiVersion : '2019-02-19' }), which makes it inconsistent with how the stripe_account option is set. The fact that it's done with a method also forces me to add an extra if statement to one of my modules, which is a small but annoying inconvenience.

Additionally, the setApiVersion() method is dangerous because it gives the impression that it is okay to set the API version at any time before calling a Stripe API. This is problematic because if a single instance of the SDK is used across multiple routes on a server (very common use case), then we are prone to one route accidentally influencing the API version used by other, separate routes. You have to be very careful to first read the existing API version, then set the new API version, then call the desired Stripe API, and finally set the API version back to what it was originally. Yet even then, you are prone to race conditions between routes.

I think moving this option to the constructor would be a great improvement to the SDK and that alone is probably sufficient. However, if people are relying on the ability to override the version for individual API calls, then this option should be supported on each API method.

[ob-stripe]

Hi @sholladay, thanks for this feedback!

I agree with you that properties like the API key or API version should only be settable at creation time and be immutable afterwards (though still overridable on a per-request basis).

Note that you can actually set the API version on a per-request basis, by passing stripe_version in the additional options: https://github.com/stripe/stripe-node/wiki/Passing-Options#options. (But this API is not great, using snake case is surprising since the rest of the library uses camel case, plus stripe_version is inconsistent with setApiVersion.)

@rattrayalex-stripe Any thoughts / opinions on the above?

[sholladay]

you can actually set the API version on a per-request basis

Ah, thanks for pointing that out! I had done a rudimentary search for variations of apiVersion and came up empty.

Well since that's already implemented, removing setApiVersion() will hopefully be a less drastic change. :)

[rattrayalex-stripe]

Hi @sholladay – thanks for the feedback! This is great; please send more thoughts like this in the future!

Definitely +1 to being able to set things like this at initialization time. I was probably thinking this API, curious for your thoughts:

const stripe = new Stripe(process.env.STRIPE_SECRET_KEY, {
  stripe_version: '2019-01-01', // etc...
});

We should probably also move the docs for passing options from a Wiki to the README...

cc also @paulasjes-stripe

[sholladay]

stripe_version is consistent with stripe_account, so 👍 in that regard. I do think these should ultimately use camel case and be renamed to not have a redundant stripe_ prefix. It's more idiomatic for JavaScript. I tend to use camelcase-keys and snakecase-keys to hide the snake casing from downstream systems when using the SDK.

move the docs for passing options from a Wiki to the README

Yeah, I agree. I've disabled wikis on all of my own projects and moved everything to .md files in the repos. More discoverable that way, plus changes live with the rest of the commit history.

[paulasjes-stripe]

I'd like to bikeshed on the naming a bit and suggest:

const stripe = new Stripe(process.env.STRIPE_SECRET_KEY, {
  apiVersion: '2019-01-01', // etc...
});

Does it make sense to use that object as a generic config object and do things like setMaxNetworkRetries in there?

const stripe = new Stripe(process.env.STRIPE_SECRET_KEY, {
  apiVersion: '2019-01-01', // etc...
  maxNetworkRetries: 5,
});

[rattrayalex-stripe]

Ah, I hadn't read carefully earlier. Definitely agreed with this:

const stripe = new Stripe(process.env.STRIPE_SECRET_KEY, {
  apiVersion: '2019-01-01',
  maxNetworkRetries: 5,
  // all other config options
});

I also like the idea of moving to camel case on each request override, like this:

stripe.charges.refund(chargeId, {
  amount: 500,
}, {
  stripeAccount: connectedAccountId,
  apiVersion: '2019-01-01', 
});

Thanks, Seth and Paul!

[paulasjes-stripe]

We talked about this internally and we've split it into a few tasks:

1. Move setApiVersion to be set via a config object in the Stripe object initialization, with the intention of deprecating stripe.setApiVersion
2. Move other setters like setMaxNetworkRetires to be set via the config object
3. Change maxNetworkRetries so it can be set on a per-request basis as well as a global setting

The config object in task 1 will need to be quite strict about what it accepts, throwing errors if it contains something unexpected.

[paulasjes-stripe]

Fixed in #703