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
Use @mswjs/interceptors for mocking - WIP #2517
base: main
Are you sure you want to change the base?
Conversation
Sorry I'm not sure when I get to review it, I'll try to find time on Saturday. If we manage to replace nock's on interceptors with the one from @mswjs while keeping all tests passing then I'm all for it |
lib/intercept.js
Outdated
return overriddenRequest(options, callback) | ||
const req = convertFetchRequestToClientRequest(request); | ||
req.on('response', response => { | ||
request.respondWith(new Response('test', { status: 200 })) |
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.
Nitpick: we'd want to read the response stream to the ReadableStream
of the response
instance and call request.respondWith()
on the end
event of the original response here.
As I mentioned in the issue, let me know if exporting this existing utility from Interceptors would help here. I think it would.
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.
Do you have an idea how we can wait for the nockResponse end
event? I use a promise, but maybe you had something else in mind.
As I mentioned in the issue, let me know if exporting this existing utility from Interceptors would help here. I think it would.
Thanks!! For now, I copied the file. We can update this later.
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 this case, we can rely on the internal http.ClientRequest
implementation to guarantee the order of event execution.
const stream = new Readable()
const fetchResponse = new Response(stream, {
status: response.statusCode,
statusText: response.statusMessage,
headers: response.headers // pseudo-code
})
response.on('data', (chunk) => stream.write(chunk))
response.on('end', () => {
stream.finish()
request.respndWith(fetchResponse)
})
This is a gist. If you want the actual implementation example, take a look at this function.
lib/intercept.js
Outdated
) | ||
overrideClientRequest() | ||
const interceptor = new BatchInterceptor({ | ||
name: 'my-interceptor', |
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.
Giving this a meaningful name may improve the logging output, afaik. Maybe name: 'nock-interceptor'
?
lib/intercept.js
Outdated
host: url.hostname, | ||
port: url.port || (url.protocol === 'https:' ? 443 : 80), | ||
path: url.pathname + url.search, | ||
headers: fetchRequest.headers, |
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 think Node expects OutgoingHeaders
here, which is not compatible with the Headers
instance. You should be fine doing this:
headers: Object.fromEntries(fetchRequest.headers.entries())
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.
great catch! 🙏
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.
Just be cautious about the compilation target here. I'm not sure what's the support range is for Nock, but .fromEntries()
and .entries()
methods may not exist on super old versions of ECMAScript and Node.
lib/create_response.js
Outdated
* @param {IncomingMessage} message | ||
*/ | ||
function createResponse(message) { | ||
const readable = new ReadableStream({ |
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.
Looks like the version of Node Nock uses doesn't have the ReadableStream
global. I'd double-check if this isn't the linter's problem as the first measure.
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.
Sometimes the linter doesn't play well with relatively newly added globals. This can be modified in the linter's globals
key, if talking about ESLint.
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.
Thanks!
I think we can fix this after we will solve the got
error. Most of Nock's tests are based on it and I'm eager to solve it and run all tests to find cases that we haven't covered yet.
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 will try to look into the got
issue this week based on my availability. Right now, the best next step is to reliably reproduce it in an automated test. I've already written one (see https://github.com/mswjs/interceptors/pull/432?notification_referrer_id=NT_kwDOAOSmz7M3NzYwMzQ3MDA1OjE0OTg0OTEx¬ifications_query=is%3Aunread#issuecomment-1724139617) but you've provided some input that I need to reflect in the test.
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've fixed the got
issue, would appreciate a quick review of the pull request!
@kettanaito Maybe you have an idea of how I can propagate errors from the interceptors? Specifically, I'm trying to fix this test. I tried something like: interceptor.on('request', function ({ request, requestId }) {
...
const nockRequest = convertFetchRequestToClientRequest(request);
nockRequest.on('error', error => {
request.respondWith(new Response('put here the real error'))
resolve()
})
...
}) But it stuck. |
@mikicho, you can throw in the request listener but that will be considered a request error. Try it, see if it produces the result you need. interceptor.on('request', () => {
nockRequest.on('error', (error) => {
throw error
})
}) |
@kettanaito |
Oh, I see. Try responding with request.respondWith(Response.error(YOUR_ERROR_MESSAGE)) This will instruct Interceptors to treat this as an intentional error, and it will forward it to the appropriate error handling logic based on the request module, like emitting the |
Just want to say thank you @mikicho and @kettanaito for working on this 💐💝 |
@kettanaito Thank you for your willingness to help and I apologize for any clutter I may cause! |
@gr2m Why does Nock expose the request object via the response ("res.req") |
@gr2m Also, we currently pass the |
I believe it's not part of the public API but Node.js associates the OutgoingMessage with the IncomingMessage that way. I do recall seeing something like that recently during testing, so you may be seeing the same thing. It's highly likely Node's internals. |
@mikicho your personal well being and your loved ones always come first ❤️ |
Hey @mikicho are you going to progress with your PR? Amazing work btw. 💪 It's a blocker for us right now - because of usage of swagger-js which uses undici from this version Looking forward for this PR to be ready-to-go and merged soon 🚀 |
A quick update on my side: I'm on holiday until January. I will come back then and will plan all the open issues/pull requests to @mswjs/interceptors into my workflow so to unblock this feature. Huge thanks for improving the library! |
@piotrm221 Thanks! :) There are some things to do in both |
@mikicho @kettanaito Thank you for continuing to work on this! Would it help if I onboarded you as a @nock maintainer? I'm having a lot going on at work and life right now. I expect it to calm down in the 2nd half of the year, I can't wait to find more time for Open Sourcering again. But until then, I don't want to become a bottleneck, and native fetch support is essential if nock is to have a future |
@gr2m, I will do my best to help with what I can. I'm back from the holidays and slowly getting back to open source. I have "Nock + Interceptors" on my roadmap, we will get it done. Your test suite has already pointed out multiple shortcomings of our implementation, so it's essential for MSW's future as well! I think making @mikicho a maintainer would certainly ease the organization around merging and publishing this change 💯 |
I think there is not a lot of work to make this happen, and this work contributes to both Nock and MSW. |
@mikicho, I will have time. I'm a bit busy right now but this change is a goal I want to see finished. I'm prioritizing it alongside my other work in MSW. I think we've addressed most of the issues related to the 2.0 release of the library, so I can switch to other things, this change included! Need to go through all your open issues and pull requests. If you can join my Discord we could iterate on this faster. My GitHub notifications tend to get overwhelming. |
Thanks to @kettanaito's work, we now have a socket-based interceptor, which should be more future-proof and more compatible with Node. |
Why?
nock
work nativefetch
#2397.@mswjs/interceptors
supports bothhttp.get/request
andfetch
.@mswjs/interceptors
is inspired byNock
and written in a modern way for maximum compatibility.closes #2397, closes #2183
Parity Gaps
Nock is a mature and battle-proof project and used by a lot of developers over the years. therefore, Nock covers many real-life scenarios and takes care of a LOT of edge cases.
@mswjs/interceptors
is great, but it still has some gaps to cover in order to reach Nock's level of maturity and ability to cover its wide test suite.I adjust as much as I can Nock's code to
@mswjs/interceptors
logic and current capabilities and opened tickets in the repo to track issues and feature requests:Topics to discuss about
During the work, I gathered a few topics we need to discuss further. I'll open dedicated issue(s) for them in Nock's repo.
CC @gr2m @kettanaito