Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
mikicho opened this issue Aug 31, 2024 · 6 comments · May be fixed by #676
Open

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

mikicho opened this issue Aug 31, 2024 · 6 comments · May be fixed by #676

Comments

@mikicho
Copy link
Contributor

mikicho commented Aug 31, 2024

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
Copy link

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 mikicho mentioned this issue Oct 1, 2024
Open
2 tasks
@mikicho
Copy link
Contributor Author

mikicho commented Oct 1, 2024

@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
Copy link
Member

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
Copy link
Contributor Author

mikicho commented Oct 26, 2024

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

@mikicho
Copy link
Contributor Author

mikicho commented Oct 26, 2024

@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
Copy link
Member

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?

@mikicho mikicho linked a pull request Nov 11, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Michael/allow custom errors node mswjs/interceptors
3 participants
@marikaner @mikicho @kettanaito