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

Warn on unknown options #465

Merged
merged 3 commits into from
May 31, 2018
Merged

Conversation

jlomas-stripe
Copy link
Contributor

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 that emitWarning() is the right way, and it might be better to just console.warn() instead. Thoughts?

r? @brandur-stripe

@jlomas-stripe jlomas-stripe force-pushed the jlomas-warn-on-unknown-options branch 3 times, most recently from 71edd0d to 5b5e921 Compare May 30, 2018 22:24
@jlomas-stripe jlomas-stripe force-pushed the jlomas-warn-on-unknown-options branch from 5b5e921 to 86892ee Compare May 30, 2018 22:25
@jlomas-stripe
Copy link
Contributor Author

OMG THEY ALL FINALLY PASSED 😂

Copy link
Contributor

@brandur-stripe brandur-stripe left a 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
Copy link
Contributor

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?

Copy link
Contributor Author

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 */
Copy link
Contributor

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.

process.nextTick(function() {
process.removeListener('warning', onProcessWarn);
})
}
}
/* eslint-enable no-console */
Copy link
Contributor

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.

@jlomas-stripe
Copy link
Contributor Author

@brandur-stripe Apparently I had no idea how eslint actually worked. 😂

// Shim `console.warn`
var _warn = console.warn;
function handleWarnings(doWithShimmedConsoleWarn, onWarn) {
/* eslint-disable no-console */
Copy link
Contributor

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.)

console.warn = _warn;
/* eslint-enable no-console */
} else {
/* eslint-disable no-inner-declarations */
Copy link
Contributor

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?

@brandur-stripe
Copy link
Contributor

@brandur-stripe Apparently I had no idea how eslint actually worked. 😂

Haha, you and me both!

ptal @jlomas-stripe

@jlomas-stripe
Copy link
Contributor Author

@brandur-stripe I ... totally missed those. 🤦‍♂️ Fixed 'em.

@brandur-stripe
Copy link
Contributor

Thanks!

@brandur-stripe brandur-stripe merged commit 906f781 into master May 31, 2018
@brandur-stripe brandur-stripe deleted the jlomas-warn-on-unknown-options branch May 31, 2018 23:35
@brandur-stripe
Copy link
Contributor

Released as 6.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants