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

Avoid agent clone #125

Merged
merged 7 commits into from
Nov 11, 2020
Merged

Avoid agent clone #125

merged 7 commits into from
Nov 11, 2020

Conversation

emmansun
Copy link
Contributor

@emmansun emmansun commented Nov 5, 2020

Do not use cloned agent
really not clone, reuse the originally agent by default, can specify one in retry strategy, even remove it via null value
Copy link
Owner

@FGRibreau FGRibreau left a comment

Choose a reason for hiding this comment

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

Would it be possible to remplace "_.cloneDeep" into our own "clone" functions that would have the responsability to NOT clone the agent? It seems to me it would be cleaner, don't you think?

use own clone function
@emmansun
Copy link
Contributor Author

emmansun commented Nov 6, 2020

What's the risk if we do not clone the options?

@FGRibreau
Copy link
Owner

FGRibreau commented Nov 6, 2020

"Never" use for(let a in obj), see:

a = {a:true};
a.__proto__.b = 3;
for (let key in a) {
console.log(key);}

See: http://bonsaiden.github.io/JavaScript-Garden/#object.hasownproperty

Copy link
Owner

@FGRibreau FGRibreau left a comment

Choose a reason for hiding this comment

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

Near completion, a test for this use-case is missing and then I will merge it 👌

t.strictEqual(200, response.statusCode);
t.strictEqual(agent, this.options.agent);
t.deepEqual(cloneable, this.options.cloneable);
t.notEqual(cloneable, this.options.cloneable);
Copy link
Owner

Choose a reason for hiding this comment

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

Hum the test cannot pass with this:

t.deepEqual(cloneable, this.options.cloneable);
t.notEqual(cloneable, this.options.cloneable);

Also, no need to add a strategy, the idea is too keep the test as small as possible with only whats required to run it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added clone.test.js

Copy link
Owner

@FGRibreau FGRibreau left a comment

Choose a reason for hiding this comment

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

Need test update

const http = require('http');
const t = require('chai').assert;

describe('Clone option function', function () {
Copy link
Owner

Choose a reason for hiding this comment

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

_cloneOptions(this.options)

output is available as the retryStrategy input. Instead of relying on a hack like rewire why not create a test, that specify :

  • an agent as option
  • its own retryStrategy, that we will use to check that agent was not duplicated

it should be a lot simpler, won't require rewire, have a simpler setup phase and only 2 assertions will be required (one for the agent the other for the other field that was cloned)

@FGRibreau FGRibreau merged commit bb2f12b into FGRibreau:master Nov 11, 2020
@FGRibreau
Copy link
Owner

Thanks @emmansun ! Released in v4.1.2!

@emmansun
Copy link
Contributor Author

Thank @FGRibreau for your patience and help!

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.

None yet

2 participants