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

Nexmo constructor changes of given options object #295

Closed
Snelius30 opened this issue Apr 1, 2020 · 5 comments · Fixed by #311
Closed

Nexmo constructor changes of given options object #295

Snelius30 opened this issue Apr 1, 2020 · 5 comments · Fixed by #311
Assignees
Labels
bug A defect in the code Good First Issue Good First Issue
Milestone

Comments

@Snelius30
Copy link

Here https://github.com/Nexmo/nexmo-node/blob/master/src/index.js#L103
must be copy of an options object instead of direct usage.

@AlexLakatos
Copy link
Contributor

Can you explain why it must be a copy, please?

@Snelius30
Copy link
Author

Cuz it's changing passed object. Otherwise i need to make a copy to pass the options object which wrong, no one libs should change passed args without return needs.

@AlexLakatos
Copy link
Contributor

That is not changing the options object though. It's changing some other class properties, that aren't passed in.

@Snelius30
Copy link
Author

Snelius30 commented Apr 2, 2020

I can wrong with code line. Just check this out

const Nexmo = require('nexmo');
const options = {};
const nexmo = new Nexmo({ apiKey: 'testKey', apiSecret: 'testSecret' }, options);
console.log(options);

The options object will changed with internal data.

@AlexLakatos
Copy link
Contributor

OK. Found the culprit: https://github.com/Nexmo/nexmo-node/blob/master/src/Nexmo.js#L40
Changing that to this.options = Object.assign({}, options); should do the trick. I'll get on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect in the code Good First Issue Good First Issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants