Conversation
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
FGRibreau
left a comment
There was a problem hiding this comment.
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
|
What's the risk if we do not clone the options? |
|
"Never" use See: http://bonsaiden.github.io/JavaScript-Garden/#object.hasownproperty |
FGRibreau
left a comment
There was a problem hiding this comment.
Near completion, a test for this use-case is missing and then I will merge it 👌
test/strategies.test.js
Outdated
| t.strictEqual(200, response.statusCode); | ||
| t.strictEqual(agent, this.options.agent); | ||
| t.deepEqual(cloneable, this.options.cloneable); | ||
| t.notEqual(cloneable, this.options.cloneable); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
added clone.test.js
This reverts commit 88aa12e.
| const http = require('http'); | ||
| const t = require('chai').assert; | ||
|
|
||
| describe('Clone option function', function () { |
There was a problem hiding this comment.
_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)
|
Thanks @emmansun ! Released in v4.1.2! |
|
Thank @FGRibreau for your patience and help! |
#124