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.

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

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

Closed
wants to merge 4 commits into from

Conversation

kettanaito
Copy link
Member

*
* @see https://github.com/nodejs/node/blob/c2cd74453e7d2794ad81cab63e68371e08bad04f/lib/_http_outgoing.js#L1161
*/
this.end('')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct (see the comment above).

We don't distinguish between writing headers and body in the interceptor, we only tap into the write() and end() methods.

@kettanaito kettanaito changed the title fix(ClientRequest): implement "flushHeaders()" feat(ClientRequest): implement "flushHeaders()" Sep 23, 2023
@kettanaito
Copy link
Member Author

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

mikicho commented Sep 23, 2023

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

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

mikicho commented Oct 5, 2023

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

Closing in favor of #515. Relying on the Socket will automatically support .flushHeaders().

@kettanaito kettanaito closed this Apr 15, 2024
@kettanaito kettanaito deleted the fix/req-flush-headers branch April 15, 2024 21:32
@kettanaito
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support http.request flushHeaders
2 participants