mswjs / interceptors

Low-level network interception library.
https://npm.im/@mswjs/interceptors
MIT License
537 stars 123 forks source link

feat(ClientRequest): implement "flushHeaders()" #442

Closed kettanaito closed 5 months ago

kettanaito commented 11 months ago
kettanaito commented 11 months ago

I've moved forward with the implementation for this. Right now, the main challenge is to distinguish between these two scenarios:

  1. .end() + request without any body. In this case, request.body must be null in the listener.
  2. .flushHeaders() + .write() + .end() (flushed request with body). In this case, request.body should be a ReadableStream eventually populated by the .write()/.end() calls.

Since the request event is now emitted in .flusHeaders(), we somehow need to know if this request is going to have any body or not so its Fetch API representation has the correct body property set. That proves to be a bit problematic.

@mikicho, maybe you've got some thoughts on this?

mikicho commented 11 months ago

This is interesting. I didn't know about the flushHeaders function either, so I'm unfamiliar with its usage patterns.

From what I see, Nock runs the interceptors logic, and don't wait for further write requests.

After reading your insights and Node code. I think Nock implemented it incorrectly, flushHeaders shouldn't kickstart the request, it just flush headers. From my understanding, in the context of a mocking library, we don't care about it. we just need wait to until the end event and then flush all data as usual.

So, I think we need to do nothing on flushHeaders. maybe patch the function and save the headers if the original function removes it from the buffer.

WDYT?

kettanaito commented 11 months ago

flushHeaders shouldn't kickstart the request, it just flush headers.

That's the same thing! Node.js buffers the request body entirely before sending it to the server with .end(). If you imagine sending a really large request to the server, you may want to start streaming it to the server early, while the request body is still being written. That's precisely what .flushHeaders() does.

Also an important notice that "headers" here refers to the HTTP message headers, which is this:

GET /url HTTP/1.0
content-type: text/plain;charset=UTF-8
some-other-header: value

Basically, the start of the HTTP message + the actual headers. This is why flushHeaders() means starting a request: that first part of the HTTP message is enough to kickstart a request.

mikicho commented 11 months ago
const req = http.request('http://httpstat.us/200')
const emitSpy = sinon.spy(req, 'emit')
req.setHeader('my-header', 'tes')
req.flushHeaders()

req.on('response', res => {
  const emitSpyRes = sinon.spy(res, 'emit')
  res.on('data', () => {})
  res.on('end', () => {
    console.log(emitSpy.getCalls().map(e => e.firstArg))
    console.log(emitSpyRes.getCalls().map(e => e.firstArg))
    done()
  })
  req.end()
})

This prints:

[ 'socket', 'response', 'prefinish', 'finish' ] [ 'resume', 'data', 'readable', 'end' ]

If I remove the req.end() the process "hang" (which is normal) and print:

[ 'socket', 'response' ] [ 'resume', 'data', 'readable', 'end' ]

So it seems like we want to run the interceptor (request) and return mockedResponse if it exists.

kettanaito commented 5 months ago

Closing in favor of https://github.com/mswjs/interceptors/pull/515. Relying on the Socket will automatically support .flushHeaders().

kettanaito commented 2 months ago

Released: v0.32.0 🎉

This has been released in v0.32.0!

Make sure to always update to the latest version (npm i @mswjs/interceptors@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.