refactor: Refactor RequestOverrider#1674
Conversation
This makes RequestOverrider into a class and breaks its logic up into several shorter methods. There is more that can be done here, though this seems like a meaningful place to pause.
|
|
||
| if (options.headers) { | ||
| // We use lower-case header field names throughout Nock. | ||
| options.headers = common.headersFieldNamesToLowerCase(options.headers) |
There was a problem hiding this comment.
Moved to the constructor.
| // changes affecting the user so we use a clone of the object. | ||
| options = _.clone(options) | ||
|
|
||
| response.req = req |
There was a problem hiding this comment.
Moved to attachToReq().
|
|
||
| // We may be changing the options object and we don't want those | ||
| // changes affecting the user so we use a clone of the object. | ||
| options = _.clone(options) |
There was a problem hiding this comment.
Moved to the constructor.
| if (req.socket.authorized) { | ||
| req.socket.emit('secureConnect') | ||
| } | ||
| }) |
There was a problem hiding this comment.
All moved to attachToReq().
| } | ||
| }) | ||
|
|
||
| req.write = function(buffer, encoding, callback) { |
There was a problem hiding this comment.
These implementations were moved to e.g. handleWrite, which in attachToReq() are set to override e.g. req.write.
| const end = function() { | ||
| debug('ending') | ||
| ended = true | ||
| let requestBody, responseBody, responseBuffers |
There was a problem hiding this comment.
requestBody is now a local variable in end(). { responseBody, responseBuffers } become named parameters to continueWithResponseBody().
|
|
||
| return continueWithResponseBody(responseBody) | ||
|
|
||
| function continueWithFullResponse(fullReplyResult) { |
There was a problem hiding this comment.
Moved to continueWithFullResponse() (a method).
| continueWithResponseBody(responseBody) | ||
| } | ||
|
|
||
| function continueWithResponseBody(responseBody) { |
There was a problem hiding this comment.
Moved to continueWithResponseBody() (a method).
|
|
||
| process.nextTick(respond) | ||
|
|
||
| function respond() { |
There was a problem hiding this comment.
Moved to respondUsingInterceptor() (a method).
| _respond() | ||
| } | ||
|
|
||
| function _respond() { |
There was a problem hiding this comment.
Now a local function inside respondUsingInterceptor().
|
I have reservations on losing all the git history for the overrider. |
|
I could rename it back in this PR (and then squash it). I think the file will still be rewritten on this commit, though, so I'm not sure if it will make of a difference. |
|
GitHub is noticing the rename now, so I guess this way it will be detected as a rename when it's actually renamed. |
|
Thanks for reviewing, Matt! |
|
🎉 This PR is included in version 11.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This makes RequestOverrider into a class and breaks its logic up into several shorter methods. The goal is to make this code easier to understand, and easier to change. There is more that can be done here, though this seems like a meaningful place to pause.
This makes RequestOverrider into a class and breaks its logic up into several shorter methods. The goal is to make this code easier to understand, and easier to change. There is more that can be done here, though this seems like a meaningful place to pause.
This makes RequestOverrider into a class and breaks its logic up into several shorter methods. The goal is to make this code easier to understand, and easier to change. There is more that can be done here, though this seems like a meaningful place to pause.
This makes RequestOverrider into a class and breaks its logic up into several shorter methods. The goal is to make this code easier to understand, and easier to change. There is more that can be done here, though this seems like a meaningful place to pause.
This makes RequestOverrider into a class and breaks its logic up into several shorter methods. The goal is to make this code easier to understand, and easier to change. There is more that can be done here, though this seems like a meaningful place to pause.
Git and GitHub are not detecting this as a rename, though it is probably just as well, as the diff would be hard to read. I think the best way to review this is by first reading the new implementation to see where things ended up, then reviewing the old one chunk by chunk. I'll leave some comments in the old code indicating where it ended up in the new code.