Throw an error if options are provided inside the data argument #306

jlomas-stripe · 2017-02-16T20:43:36Z

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 · 2017-02-17T22:26:06Z

Oops. :)

r? @brandur-stripe

jlomas-stripe · 2017-02-20T17:42:55Z

Relates to #307.

brandur-stripe · 2017-02-23T16:51:03Z

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 · 2017-02-23T17:04:13Z

@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 · 2017-02-23T17:06:57Z

@jlomas-stripe Awesome! Thank you :)

jlomas-stripe · 2017-08-08T21:46:43Z

@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 · 2017-08-08T21:51:20Z

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 · 2017-08-08T21:59:22Z

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 · 2017-08-08T22:05:04Z

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 · 2017-08-08T22:10:21Z

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

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

brandur-stripe · 2017-08-10T17:03:32Z

@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 · 2017-08-10T17:25:15Z

@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 · 2017-08-22T14:53:20Z

@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 · 2017-08-25T18:01:39Z

lib/utils.js

+      return OPTIONS_KEYS.indexOf(key) > -1;
+    });
+
+    if (argKeys.length > optionKeysInArgs.length) {


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

Yes. Yes it would. 😂

brandur-stripe · 2017-08-25T18:03:53Z

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 · 2017-09-22T23:05:27Z

@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 · 2017-09-25T18:00:16Z

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

brandur-stripe · 2017-09-25T18:03:46Z

Released as 5.1.0 along with a few other minor things.

jlomas-stripe force-pushed the jlomas-options-in-data branch from cf658d2 to 2381c2e Compare February 16, 2017 20:45

mikaelcabot mentioned this pull request Mar 13, 2017

stripe.charges.list #311

Closed

jlomas-stripe force-pushed the jlomas-options-in-data branch from 2381c2e to 087dca4 Compare August 8, 2017 21:18

jlomas-stripe changed the title ~~Allow for a combined options/data object without losing the data~~ Throw an error if options are provided inside the data argument Aug 8, 2017

jlomas-stripe force-pushed the jlomas-options-in-data branch 2 times, most recently from fc7c415 to 82305e4 Compare August 8, 2017 21:22

brandur-stripe mentioned this pull request Aug 22, 2017

Bad options keys; should we throw? #375

Closed

brandur-stripe reviewed Aug 25, 2017

View reviewed changes

jlomas-stripe added 2 commits September 22, 2017 15:45

Throw an error if options are provided inside the data argument

f1a65ef

Switch from throwing to console.warn()ing on options in args

4ec6955

jlomas-stripe force-pushed the jlomas-options-in-data branch from 82305e4 to 4ec6955 Compare September 22, 2017 23:04

brandur-stripe merged commit 708efe3 into master Sep 25, 2017

brandur-stripe deleted the jlomas-options-in-data branch September 25, 2017 18:00

jlomas-stripe mentioned this pull request Oct 3, 2017

Fix the comparison for the args/options warnings #394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw an error if options are provided inside the data argument #306

Throw an error if options are provided inside the data argument #306

jlomas-stripe commented Feb 16, 2017

jlomas-stripe commented Feb 17, 2017

jlomas-stripe commented Feb 20, 2017

brandur-stripe commented Feb 23, 2017 •

edited

Loading

jlomas-stripe commented Feb 23, 2017

brandur-stripe commented Feb 23, 2017

jlomas-stripe commented Aug 8, 2017

remi-stripe commented Aug 8, 2017

jlomas-stripe commented Aug 8, 2017 •

edited

Loading

remi-stripe commented Aug 8, 2017 •

edited

Loading

jlomas-stripe commented Aug 8, 2017 •

edited

Loading

brandur-stripe commented Aug 10, 2017 •

edited

Loading

jlomas-stripe commented Aug 10, 2017 •

edited

Loading

jlomas-stripe commented Aug 22, 2017

brandur-stripe Aug 25, 2017

jlomas-stripe Sep 22, 2017

brandur-stripe commented Aug 25, 2017

jlomas-stripe commented Sep 22, 2017

brandur-stripe commented Sep 25, 2017

brandur-stripe commented Sep 25, 2017

Throw an error if options are provided inside the data argument #306

Throw an error if options are provided inside the data argument #306

Conversation

jlomas-stripe commented Feb 16, 2017

jlomas-stripe commented Feb 17, 2017

jlomas-stripe commented Feb 20, 2017

brandur-stripe commented Feb 23, 2017 • edited Loading

jlomas-stripe commented Feb 23, 2017

brandur-stripe commented Feb 23, 2017

jlomas-stripe commented Aug 8, 2017

remi-stripe commented Aug 8, 2017

jlomas-stripe commented Aug 8, 2017 • edited Loading

remi-stripe commented Aug 8, 2017 • edited Loading

jlomas-stripe commented Aug 8, 2017 • edited Loading

brandur-stripe commented Aug 10, 2017 • edited Loading

jlomas-stripe commented Aug 10, 2017 • edited Loading

jlomas-stripe commented Aug 22, 2017

brandur-stripe Aug 25, 2017

Choose a reason for hiding this comment

jlomas-stripe Sep 22, 2017

Choose a reason for hiding this comment

brandur-stripe commented Aug 25, 2017

jlomas-stripe commented Sep 22, 2017

brandur-stripe commented Sep 25, 2017

brandur-stripe commented Sep 25, 2017

brandur-stripe commented Feb 23, 2017 •

edited

Loading

jlomas-stripe commented Aug 8, 2017 •

edited

Loading

remi-stripe commented Aug 8, 2017 •

edited

Loading

jlomas-stripe commented Aug 8, 2017 •

edited

Loading

brandur-stripe commented Aug 10, 2017 •

edited

Loading

jlomas-stripe commented Aug 10, 2017 •

edited

Loading