🚧 Gregor's code review and refactor #2252
Conversation
```js
const got = require('got')
const intercept = require('./modules/node-intercept')
run()
async function run() {
const reset = intercept()
console.log('Intercepted:')
console.log(await got('https://example.com').text())
// logs "Hello, world!"
console.log('\n\nNot Intercepted:')
reset()
console.log(await got('https://example.com').text())
// logs HTML from example.com
}
```
…thout arguments. Check if response callback is set before using
…logic" This reverts commit ec8fb81.
…methods are called in case a request is not intercepted
| @@ -85,7 +85,7 @@ describe('Direct use of `ClientRequest`', () => { | |||
|
|
|||
| it('should throw an expected error when creating with empty options', () => { | |||
| expect(() => new http.ClientRequest()).to.throw( | |||
| 'Creating a ClientRequest with empty `options` is not supported in Nock' | |||
| 'Making a request with empty `options` is not supported in Nock' | |||
There was a problem hiding this comment.
Technically a breaking change, but ... 🤷🏼❓
| done() | ||
| } | ||
| ) | ||
| request.end() |
tests/test_nock_lifecycle.js
Outdated
| @@ -117,7 +117,7 @@ describe('Nock lifecycle functions', () => { | |||
| const req = http.request('http://example.test', () => { | |||
| done() | |||
| }) | |||
| req.once('socket', () => { | |||
| req.once('connect', () => { | |||
There was a problem hiding this comment.
I'll try to restore the original behavior so that this test passes
| // it is not intercepted has no longer access to the original `http.{get,request}` methods. | ||
| // This will become obsolete when don't overwrite these methods at all and instead | ||
| // hook into the `http.ClientRequest` constructor, as the `mitm` package does. | ||
| it.skip('TODO! when http.get and http.request have been overridden before nock overrides them, http.get calls through to the expected method', async () => { |
There was a problem hiding this comment.
I'll create follow up issue for these, I'd consider them known bugs until we finish the refactoring that no longer requires a custom http.ClientRequest implementation
| it('when http.get and http.request have been overridden before nock overrides them, http.request calls through to the expected method', async () => { | ||
| // Skipping for now because the new way a real request gets sent out in case | ||
| // it is not intercepted has no longer access to the original `http.{get,request}` methods. | ||
| // This will become obsolete when don't overwrite these methods at all and instead | ||
| // hook into the `http.ClientRequest` constructor, as the `mitm` package does. | ||
| it.skip('TODO! when http.get and http.request have been overridden before nock overrides them, http.request calls through to the expected method', async () => { |
There was a problem hiding this comment.
I'll create follow up issue for these, I'd consider them known bugs until we finish the refactoring that no longer requires a custom http.ClientRequest implementation
Is the goal still to use mitm, or to switch to @gr2m/node-net-interceptor? I was looking also at https://mswjs.io/docs/ and see they use https://github.com/mswjs/interceptors. I don't know if collaboration makes sense to reduce workload. Their interceptor does not do socket-level |
The plan is to use these
Maybe? We need to decompose nock into smaller modules first, that will enable to swap out interceptors and maybe use the ServiceWorkers one from mswjs, but I haven't looked into that yet, there is still lots of work left to do. |
Not much to see here yet, I'm still messing around.
My goal is to change the codebase into ways that there are clear separations of concerns based on folders, with the goal to move the different folders into separate modules. I'm current working on the intercept part.
You can learn more at #2247
Remaining TODOs
common.headersFieldNamesToLowerCasewe no longer coverthrowOnDuplicatenot being truthyMitmor a similar module for intercepting requests instead of overwritinghttp.ClientRequestentirelyNon-breaking changes
I had to add these two methods but I think we shouldn't mention them because they will be removed again once we stop overwriting the entire
http.ClientRequestclass and instead hook into it by patchinghttp.ClientRequest.prototype.onSocketinterceptedClientRequest.nockSendRealRequest()interceptedClientRequest.nockGetRequestBodyChunks()Breaking changes
Before, the intercept logic occurred directly in the overridden
http.ClientRequestconstructor, after thesocketevent was emitted. Now, the intercept logic is only called if an actual request is being sent (after theconnectevent was emitted)Before
After
When a request is not intercepted, we call the interal
request.nockSendRealRequest()method provided by the new intercept module. When that happens, the real request is sent out, but the orginalhttp.{get,request}methods are not called. The propor fix to this breaking change is to not overridehttp.{get,request}andhttp.ClientRequestin the first place and instead hook into thehttp.ClientRequestconstructor, asmitmdoes.