-
Notifications
You must be signed in to change notification settings - Fork 751
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
Warn on unknown options
#465
Conversation
71edd0d
to
5b5e921
Compare
5b5e921
to
86892ee
Compare
OMG THEY ALL FINALLY PASSED 😂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice @jlomas-stripe.
I also fixed some tests that weren't running properly (because they were async but didn't include a done),
Man, that is a massive footgun. Thanks for grabbing those!
I'm ... also not sure that emitWarning() is the right way, and it might be better to just console.warn() instead. Thoughts?
I like it.
lib/utils.js
Outdated
@@ -75,8 +75,8 @@ var utils = module.exports = { | |||
// (the first being args and the second options) and with known | |||
// option keys in the first so that we can warn the user about it. | |||
if (optionKeysInArgs.length > 0 && optionKeysInArgs.length !== argKeys.length) { | |||
console.warn( // eslint-disable-line no-console | |||
'Stripe: Options found in arguments (' + optionKeysInArgs.join(', ') + '). Did you mean to pass an options ' + | |||
emitStripeWarning( // eslint-disable-line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just call it emitWarning
— I don't think we need the "Stripe" in there.
Also, now that there's no more console call here, can we remove the eslint-disable-line
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and yes. :)
lib/utils.js
Outdated
|
||
function emitStripeWarning(warning) { | ||
if (typeof process.emitWarning !== 'function') { | ||
/* eslint-disable no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just make sure this lint doesn't need to be re-enabled after the console call? (I'm honestly not sure.) It might be better just to use the single line disable eslint-disable-line
like we had above.
test/utils.spec.js
Outdated
process.nextTick(function() { | ||
process.removeListener('warning', onProcessWarn); | ||
}) | ||
} | ||
} | ||
/* eslint-enable no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I think you need one of these eslint-enable
to re-enable the lint if you don't use the per-line syntax.
@brandur-stripe Apparently I had no idea how eslint actually worked. 😂 |
test/utils.spec.js
Outdated
// Shim `console.warn` | ||
var _warn = console.warn; | ||
function handleWarnings(doWithShimmedConsoleWarn, onWarn) { | ||
/* eslint-disable no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another small one, but I think this eslint-disable
might have been duplicated. Can we just have one of these? (Between the ones on lines 287 and 289.)
test/utils.spec.js
Outdated
console.warn = _warn; | ||
/* eslint-enable no-console */ | ||
} else { | ||
/* eslint-disable no-inner-declarations */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here, can we either re-enable this lint after the inner declaration or use eslint-disable-line
?
Haha, you and me both! ptal @jlomas-stripe |
@brandur-stripe I ... totally missed those. 🤦♂️ Fixed 'em. |
Thanks! |
Released as 6.1.0. |
I.e., "Do the thing I said I'd do in #375".
I also fixed some tests that weren't running properly (because they were async but didn't include a
done
), and I'm ... also not sure thatemitWarning()
is the right way, and it might be better to justconsole.warn()
instead. Thoughts?r? @brandur-stripe