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.

Sign up for GitHub

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

Warn on unknown options #465

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
86892ee
console.warn on unknown options
jlomas-stripe May 30, 2018
9477bcb
Fix eslint stuff, rename function
jlomas-stripe May 31, 2018
7269a3d
Fix more eslint stuff
jlomas-stripe May 31, 2018
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
21 changes: 19 additions & 2 deletions lib/utils.js
View file
Open in desktop
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 ' +
emitWarning(
'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) {
emitWarning('Invalid options found (' + extraKeys.join(', ') + '); ignoring.');
}

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

function emitWarning(warning) {
if (typeof process.emitWarning !== 'function') {
return console.warn('Stripe: ' + warning); /* eslint-disable-line no-console */
}

return process.emitWarning(warning, 'Stripe');
}
63 changes: 51 additions & 12 deletions test/utils.spec.js
View file
Open in desktop
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,31 @@ 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;

console.warn = onWarn;
doWithShimmedConsoleWarn();

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

// Un-shim `console.warn`
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?

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

process.on('warning', onProcessWarn);

doWithShimmedConsoleWarn();

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