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
🚧 Gregor's code review and refactor #2252
base: main
Are you sure you want to change the base?
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In
common.headersFieldNamesToLowerCase
we no longer coverthrowOnDuplicate
not being truthy
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically a breaking change, but ... 🤷🏼❓
done() | ||
} | ||
) | ||
request.end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See breaking changes
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.headersFieldNamesToLowerCase
we no longer coverthrowOnDuplicate
not being truthyMitm
or a similar module for intercepting requests instead of overwritinghttp.ClientRequest
entirelyNon-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.ClientRequest
class and instead hook into it by patchinghttp.ClientRequest.prototype.onSocket
interceptedClientRequest.nockSendRealRequest()
interceptedClientRequest.nockGetRequestBodyChunks()
Breaking changes
Before, the intercept logic occurred directly in the overridden
http.ClientRequest
constructor, after thesocket
event was emitted. Now, the intercept logic is only called if an actual request is being sent (after theconnect
event 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.ClientRequest
in the first place and instead hook into thehttp.ClientRequest
constructor, asmitm
does.