Refactor request-debug to prevent mutation of module request
#18
Conversation
pedrobranco
force-pushed
the
enhancement/refactor-request-debug
branch
2 times, most recently
from
d8456da to
2513965
Compare
|
WDYT @nylen? |
|
@simov any chance of a quick +1/-1 on the concept? |
package.json
Outdated
| "main": "index.js", | ||
| "dependencies": { | ||
| "clone": "^1.0.2", | ||
| "request": "^2.75.0" |
There was a problem hiding this comment.
This library is probably due for a refactoring and update. Still, my concern with this approach is that you depend on a specific version of request, so you lose the ability to debug the version of request you're actually running in your application.
There was a problem hiding this comment.
Yes, you're right. I will put the minimum as it was before (2.30.0).
About the concept do you agree?
Edit: actually, we can support 2.19.0+.
There was a problem hiding this comment.
That doesn't really solve the problem, because it is still a hard dependency through npm and you can no longer pass a specific version of request to request-debug. Also, for example, you may have customized some settings via request.defaults that affect the operation.
There was a problem hiding this comment.
I think it would be good to remove the npm dependency and allow the user to pass in a request instance. You should be able to do that and keep the same concept of wrapping rather than modifying request. I probably won't have time to do a detailed review but the new concept seems solid to me.
There was a problem hiding this comment.
I think it would be good to remove the npm dependency and allow the user to pass in a request instance
Sure 👍
I probably won't have time to do a detailed review but the new concept seems solid to me.
Thank you for the concept ACK.
pedrobranco
force-pushed
the
enhancement/refactor-request-debug
branch
2 times, most recently
from
9c2bb88 to
6e1ea30
Compare
| - "iojs" | ||
| - "0.12" | ||
| - "0.10" | ||
| - "4.0.0" |
pedrobranco
force-pushed
the
enhancement/refactor-request-debug
branch
2 times, most recently
from
6eef63e to
ea96b3a
Compare
README.md
Outdated
| This will set up event handlers on every request performed with the `client` | ||
| variable from this point. | ||
|
|
||
| The constructor `RequestDebug` supports all options of `request` (see options [here](https://github.com/request/request#requestoptions-callback)): |
README.md
Outdated
| baseUrl: 'http://www.google.com', | ||
| json: true | ||
| }); | ||
|
|
.travis.yml
Outdated
| - "4" | ||
| - "5" | ||
| - "6" | ||
| - "6.6.0" |
There was a problem hiding this comment.
6 will alias to the latest stable of 6.x.
pedrobranco
force-pushed
the
enhancement/refactor-request-debug
branch
from
ea96b3a to
1467094
Compare
|
All comments were addressed. |
|
@simov can you take a look at this one? |
4 participants
Description
This PR has an intent of preventing changing the module
requestby reference, and to do it we create a wrapper around an instance ofrequestto be able to define adebugfunction like we have already, but, instead of changingrequestprototype functioninit, we are listening to the events ofrequestcall.This PR is a major refactor (breaking change) of
request-debug, the requirements are changed:This PR has a new test suite with the scope of testing only the logic present in
request-debug.Issues