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

Do not mutate user-supplied opts #1041

Merged
merged 1 commit into from
Oct 14, 2020

Conversation

vcheung-stripe
Copy link
Contributor

@vcheung-stripe vcheung-stripe commented Oct 13, 2020

Summary

When a user supplies opts to a function, the user's opts are mutated. This becomes a problem when a user supplies an opt that is deprecated; e.g. stripe_account instead of stripeAccount. In this scenario:

  • Before calling the function:
opts = {
  ...
  stripe_account: ...,  // user supplies a deprecated opt
  ...
}
  • After calling the function, the object is mutated:
opts = {
  ...
  stripe_account: ...,
  stripeAccount: ...,  // the function unexpectedly inserts the non-deprecated version of the opt
  ...
}

Because of this mutation, the user is warned that they have supplied both of these args, when in fact they have only supplied one.

This change makes a shallow copy of the user's opts so that this does not happen.

Motivation

fixes #1029

Test plan

  • Unit tests

@@ -1,6 +1,7 @@
'use strict';

const utils = require('./utils');
const cloneDeep = require('lodash.clonedeep');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd try to avoid the extra dependency if at all possible. Do you think it is necessary to deep clone? I haven't checked, but my guess would be we aren't doing any mutations more than a single level down and a shallow clone {...opts} would be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using shallow copying with the ... operator, the opts do get mutated, so I think we do need a deep copy. What works is JSON.parse(JSON.stringify(args)), of course this isn't the most robust way of copying. Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I may have misread your comment, I will try the shallow copy one level down and report back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shallow copying one level down works! We do not need this dependency anymore. ptal @richardm-stripe

Copy link
Contributor

@richardm-stripe richardm-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@richardm-stripe richardm-stripe merged commit a823ea8 into master Oct 14, 2020
@richardm-stripe richardm-stripe deleted the vcheung/user-supplied-opts-bug-fix branch October 14, 2020 14:00
@jacoblee93
Copy link

jacoblee93 commented Oct 27, 2020

Hey @vcheung-stripe and @richardm-stripe, we're using Node 6 this line broke our build -- Node 6 doesn't support the spread operator in objects:

const params = {...args.pop()};

Can we change this to something else? Version deprecations shouldn't be done in a minor release at any rate. I'll open a PR shortly.

@paulasjes-stripe
Copy link
Contributor

Hi @jacoblee93, we ended support for Node 6 as of version 8 of stripe-node: https://github.com/stripe/stripe-node/wiki/Migration-guide-for-v8

I admit that finding that information was harder that it should have been. I'll put up a PR shortly that clearly states our supported versions in the main readme.

@jacoblee93
Copy link

jacoblee93 commented Oct 27, 2020

Dang. Thanks for letting me know.

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.

RequestOptions with deprecated key are mutated without removing the old key
4 participants