Throw an error if options are provided inside the data argument

[jlomas-stripe]

If you include any of stripe_account, api_key or idempotency_key in the data object, that object will be treated as the options object and so only the options will be processed - i.e., the data will be discarded.

With this change, the first argument object will be stripped of the options keys and returned, allowing for both data and options to come from the same object.

Also added some Charge creation tests to ensure that this works - at least in terms of adding a Stripe-Account header.

[jlomas-stripe]

Oops. :)

r? @brandur-stripe

[jlomas-stripe]

Relates to #307.

[brandur-stripe]

Hi @jlomas-stripe, thank you so much for the hard work here and my apologies for not getting back to you about this sooner!

So I kind of feel like including stripe_account and its ilk into the same object as the rest of your data is a little bit janky, and would overall prefer that users pass it as a separate object. It probably doesn't matter, but it means that keys like api_key would forever become reserved words that couldn't be added as a parameter name on an API endpoint. I also think that it's just cleaner having them as two separate objects.

Just to think about alternatives, what would you think about instead erroring/warning when it looks like a user has accidentally mixed together data and options? It seems like this might allow for a similar level of ergonomics by giving people better feedback on what went wrong, but avoid bad patterns around name/object mangling.

[jlomas-stripe]

@brandur-stripe Very good points, and I agree on all of them. I'd also considered the error approach, so I'm totally fine with switching to that to enforce the separation.

I'll ping again when I've switched this up. :)

[brandur-stripe]

@jlomas-stripe Awesome! Thank you :)

[jlomas-stripe]

@brandur-stripe I brought this back from the edge of death, and switched it over to just throwing an error if you provide a combined options/data argument.

[remi-stripe]

Small lurk: this will need a major version change. Multiple users/integrations (over 100) incorrectly pass idempotency_key in the first hash and would instantly break with this change (assuming they use Node.js and upgrade)

[jlomas-stripe]

I will double-check that the following is true for idempotency_key, but I believe that if they currently pass ANY options in the data object, the data in that object is ignored - which is why I originally started this one - so ... their integration is already ~broken. ¯\_(ツ)_/¯

But good point. :)

[remi-stripe]

so ... their integration is already ~broken. ¯_(ツ)_/¯

Well it's not broken. It does not do what they ask, but it charges customers successfully, which is fairly different from returning an error on all requests until they fix their code.

[jlomas-stripe]

Ohhhhh. I can't read. Good point. Thanks. :)

Fully agreed: taking this approach will require a major version bump.

[brandur-stripe]

@jlomas-stripe Thanks for bringing this back to life! Seems like a pretty great improvement.

One thing I noticed is that the wiki page that you link to:
https://github.com/stripe/stripe-node/wiki/Passing-Options

Describes how to pass "data" (according to our vocabulary here), but doesn't really describe how to use "options". We sort of have the information in our API reference, but it might be a good idea to include it there too given that it's in a link. Would you be up for throwing something up there instructing how to use options like idempotency_key and stripe_account?

[jlomas-stripe]

@brandur-stripe That ... is a great point. 😂

I can absolutely tweak that page - it doesn't seem to cover off what we consider options at all anyways, and it probably should. Will take a run at that today.

EDIT: And by today I really mean hopefully today but definitely in the near future and hopefully before six months from now. ¯\_(ツ)_/¯

[jlomas-stripe]

@brandur-stripe 👋 Just wanted to follow up. I don't think there's anything left to do here, since I did update the Wiki page.

Can you take another peek when you have a chance?

[brandur-stripe]

Wouldn't this conditional be a little more intuitive if we just compare optionKeysInargs.length > 0?

[jlomas-stripe]

Yes. Yes it would. 😂

[brandur-stripe]

Nice! And thank you for updating that wiki.

One other relatively minor comment: I think I mentioned in another PR that we did essentially this exact same thing for stripe-ruby, except for that we emit a warning to stderr instead of throwing an error. In the long run, I think that throwing the error might be better, but the warning is superior for backwards compatibility (people might be doing this today by accident and will get an error on upgrade if they are). Thoughts on switching over to a warning instead?

[jlomas-stripe]

@brandur-stripe I (finally 😂) updated this to console.warn() instead of throwing; can you take a peek when you have a moment? Thanks!

[brandur-stripe]

Love it @jlomas-stripe. Thanks again for the fix!

[brandur-stripe]

Released as 5.1.0 along with a few other minor things.