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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions lib/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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. :)

'Options found in arguments (' + optionKeysInArgs.join(', ') + '). Did you mean to pass an options ' +
'object? See https://github.com/stripe/stripe-node/wiki/Passing-Options.'
);
}
Expand All @@ -98,6 +98,15 @@ var utils = module.exports = {
opts.auth = args.pop();
} else if (utils.isOptionsHash(arg)) {
var params = args.pop();

var extraKeys = Object.keys(params).filter(function(key) {
return OPTIONS_KEYS.indexOf(key) == -1;
});

if (extraKeys.length) {
emitStripeWarning('Invalid options found (' + extraKeys.join(', ') + '); ignoring.');
}

if (params.api_key) {
opts.auth = params.api_key;
}
Expand Down Expand Up @@ -195,3 +204,12 @@ var utils = module.exports = {
return obj;
},
};

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.

return console.warn('Stripe: ' + warning);
}

return process.emitWarning(warning, 'Stripe');
}
60 changes: 49 additions & 11 deletions test/utils.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,26 +106,30 @@ describe('utils', function() {
expect(utils.getDataFromArgs(args)).to.deep.equal({});
expect(args.length).to.equal(2);
});
it('ignores a hash with only options', function() {
it('ignores a hash with only options', function(done) {
var args = [{api_key: 'foo'}];

handleConsoleWarns(function() {
handleWarnings(function() {
expect(utils.getDataFromArgs(args)).to.deep.equal({});
expect(args.length).to.equal(1);

done();
}, function(message) {
throw new Error('Should not have warned, but did: ' + message);
});
});
it('warns if the hash contains both data and options', function() {
it('warns if the hash contains both data and options', function(done) {
var args = [{foo: 'bar', api_key: 'foo', idempotency_key: 'baz'}];

handleConsoleWarns(function() {
handleWarnings(function() {
utils.getDataFromArgs(args);
}, function(message) {
expect(message).to.equal(
'Stripe: Options found in arguments (api_key, idempotency_key).' +
' Did you mean to pass an options object? See https://github.com/stripe/stripe-node/wiki/Passing-Options.'
);

done();
});
});
it('finds the data', function() {
Expand Down Expand Up @@ -212,6 +216,25 @@ describe('utils', function() {
});
expect(args.length).to.equal(0);
});
it('warns if the hash contains something that does not belong', function(done) {
var args = [{foo: 'bar'}, {
api_key: 'sk_test_iiiiiiiiiiiiiiiiiiiiiiii',
idempotency_key: 'foo',
stripe_version: '2010-01-10',
fishsticks: true,
custard: true,
},];

handleWarnings(function() {
utils.getOptionsFromArgs(args);
}, function(message) {
expect(message).to.equal(
'Stripe: Invalid options found (fishsticks, custard); ignoring.'
);

done();
});
});
});

describe('arrayToObject', function() {
Expand Down Expand Up @@ -262,15 +285,30 @@ describe('utils', function() {
});

/* eslint-disable no-console */
function handleConsoleWarns(doWithShimmedConsoleWarn, onWarn) {
// 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.)

if (typeof process.emitWarning !== 'function') {
// Shim `console.warn`
var _warn = console.warn;
console.warn = onWarn;

doWithShimmedConsoleWarn();

console.warn = onWarn;
// Un-shim `console.warn`,
console.warn = _warn;
} 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?

function onProcessWarn(warning) {
onWarn(warning.name + ': ' + warning.message);
}

doWithShimmedConsoleWarn();
process.on('warning', onProcessWarn);

// Un-shim `console.warn`
console.warn = _warn;
doWithShimmedConsoleWarn();

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.