controller.errorWith ends up in the response path if the error is not Error instance

[mikicho]

interceptors expects the error to be from type Error in handleRequest, which is not required by Node.

const http = require('http')
const { ClientRequestInterceptor } = require('@mswjs/interceptors/ClientRequest')

const interceptor = new ClientRequestInterceptor()

interceptor.on('request', async function rootListener({ controller }) {
  controller.errorWith({ message: 'custom error', code: 'test' })
})
// interceptor.apply()

const req = http.request('http://example.com')

req.on('response', () => {
  console.log('response');
})
req.on('error', e => {
  console.log('error', e);
})

req.end()

If the interceptor is applied, the program prints response and throws:

/.../nock/node_modules/@mswjs/interceptors/lib/node/chunk-WTJL7BRV.js:234
if (!Reflect.has(headers, kRawHeaders)) {
               ^

TypeError: Reflect.has called on non-object
    at Reflect.has (<anonymous>)
    at getRawFetchHeaders (/home/michael/projects/js/nock/node_modules/@mswjs/interceptors/lib/node/chunk-WTJL7BRV.js:234:16)
    at MockHttpSocket.respondWith (/home/michael/projects/js/nock/node_modules/@mswjs/interceptors/lib/node/chunk-WTJL7BRV.js:488:32)
    at Object.onResponse (/home/michael/projects/js/nock/node_modules/@mswjs/interceptors/lib/node/chunk-WTJL7BRV.js:901:18)
    at handleResponse (/home/michael/projects/js/nock/node_modules/@mswjs/interceptors/lib/node/chunk-TQD7SQGP.js:100:21)
    at handleRequest (/home/michael/projects/js/nock/node_modules/@mswjs/interceptors/lib/node/chunk-TQD7SQGP.js:186:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async _ClientRequestInterceptor.onRequest (/home/michael/projects/js/nock/node_modules/@mswjs/interceptors/lib/node/chunk-WTJL7BRV.js:895:32)

Node.js v20.17.0

For real requests, it is called the error callback.

WDYT?

[marikaner]

I have the same issue currently when using @mswjs/interceptors through nock. I am currently unable to produce a reproducible example, so it is very difficult to investigate this.
Does anyone have an idea for a workaround until this is fixed? @mikicho maybe?

[mikicho]

@marikaner In order to keep this issue clean, I opened an issue in nock to track this and discuss about workaround (nock/nock#2789)

[kettanaito]

I think it should be safe to allow arbitrary error reasons. .destroy() isn't strict about this, as you've mentioned, and some other clients don't support custom error reasons at all.

The only thing, I'd be careful with the handleRequest as it tackles a bunch of scenarios and some of them are not that apparent. But all are covered with tests so any change will be verified.

I'd probably suggest to refactor handleRequest to have two internal callbacks: onResolve and onReject that are called whenever something is returned via .respondWith() or something throws during the request resolution (no matter if via throw or .errorWith()). Basically, to streamline the request handling to two paths:

• Something resolved this request to X;
• Something rejected this request with Y.

The current handleResponse and handleResponseError are too vague. Besides, there are situations when thrown errors are responses and get handled as response, which is confusing to read:

interceptors/src/utils/handleRequest.ts

Lines 74 to 77 in 472bcbe

	// Handle thrown responses.
	if (error instanceof Response) {
	return await handleResponse(error)
	}

We can likely keep the existing handleResponse/handleResponseError, maybe rename them, and use them within the newly added onResolve and onReject callbacks.

@mikicho, would you like to give this one a try?

[mikicho]

Sure! I'd love to contribute to this one...
I'm looking into it.

[mikicho]

@kettanaito I need clarification on what you have in mind.
How is it different from what we currently have?

export async function handleRequest(options) {
  const handleResponse..
  const handleResomposeError..
  ...
  const result = await until...
 if (result.error)... // a.k.a onReject
 if (result.data)... // a.ka onResolve
}

[kettanaito]

Right now we have cases where rejections can be translated to successful response handling:

interceptors/src/utils/handleRequest.ts

Line 76 in 7e62e17

return await handleResponse(error)

And successful responses translated to errors:

interceptors/src/utils/handleRequest.ts

Lines 47 to 53 in 7e62e17

	if (response instanceof Error) {
	options.onError(response)
	}
	// Handle "Response.error()" instances.
	else if (isResponseError(response)) {
	options.onRequestError(response)

This all is intended but reading it is a bit confusing. So I proposed to have higher-level onResolve and onReject callbacks within the handleRequest function to make it easier to read. It's much easier to understand "onResolve triggered with an error, so treat it as an error" than "successful response callback triggered with an error, so it's an error". Do you see my line of thinking?