fix: make sure handler is resolvable

[hoangvvo]

There was a bug that prevent await handler(req, res) to be finished. This aims to solve it by resolving when end event is called in response object.

It is possible to disable this behavior (to resolve immediately) using options.disableResponseWait.

Fixed #179

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: d9e6ff98e325553a7d7e7fc9b73c1c39f4547fbe

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[codecov[bot]]

Codecov Report

Merging #178 (d9e6ff9) into master (65cdb6a) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #178   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           78        86    +8     
=========================================
+ Hits            78        86    +8     

Impacted Files	Coverage Δ
src/index.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 65cdb6a...d9e6ff9. Read the comment docs.

[hoangvvo]

Published as beta for testing:

npm i next-connect@beta

[jakeorr]

This is what I'm testing with:

index.page.js

const handler = nc({ onError })
  .get((req, res) => {
    res.status(200).send('hello!');
  })

index.test.js

test('resolves', async () => {
  const { req, res } = createMocks({ method: 'GET', url: '/' });
  await handler(req, res);
});

Using the changes in this PR, my test's handler call is not resolving and the test times out. Maybe I'm not understanding when the "close" event occurs. Looking at your change, I think I should expect the close event to fire and the handler to resolve, please correct me if I'm wrong here.

Interestingly, if I use the disableResponseWait: true option, the handler resolves and my test completes successfully. In this case it seems like the only thing that changed is that the nc function now returns a promise (from the async function change).

There might be something here I'm not understanding. Is this the intended behaviour?

[jakeorr]

To answer one of my questions; from node-mocks-http:

NOTE: The out-of-the-box mock event emitter included with node-mocks-http is not a functional event emitter and as such does not actually emit events. If you wish to test your event handlers you will need to bring your own event emitter.

So it's not emitting the event. As a note, when I give the mock response an event emitter as shown here, it emits "end", but not "close".

[hoangvvo]

To answer one of my questions; from node-mocks-http:

NOTE: The out-of-the-box mock event emitter included with node-mocks-http is not a functional event emitter and as such does not actually emit events. If you wish to test your event handlers you will need to bring your own event emitter.

So it's not emitting the event. As a note, when I give the mock response an event emitter as shown here, it emits "end", but not "close".

According to Node document, it emits the "close" event https://nodejs.org/api/http.html#event-close_1. My unit test using super test (which spins up a real server) confirms this (see Changes). My thought is that node-mocks-http misses out on the "close" event.

But I'm okay to switch it to "end" too, but just need to make sure the event is always called as long as we "end" the response.

[hoangvvo]

Interestingly, if I use the disableResponseWait: true option, the handler resolves and my test completes successfully. In this case it seems like the only thing that changed is that the nc function now returns a promise (from the async function change).

On a side note, about this point, in both the old and new version, the function returns a promise. But the old one has a bug that prevents the promise from being resolved (#179).

When disableResponseWait is true, the promise resolved immediately (even before request middleware finish running). Only the above the test (which does not assert anything) works. If you asserts headers, body, etc, it will end up incorrectly.

[jakeorr]

Thanks for sharing all the details. What you talked about above make sense. I agree that under normal circumstances listening to the request "close" event would be fine. It would be nice for this change to listen on the "end" event just to have it work with node-mocks-http, as long as that doesn't cause other issues. I opened an issue to see what the maintainers think about emitting "close".

[hoangvvo]

Thanks for sharing all the details. What you talked about above make sense. I agree that under normal circumstances listening to the request "close" event would be fine. It would be nice for this change to listen on the "end" event just to have it work with node-mocks-http, as long as that doesn't cause other issues. I opened an issue to see what the maintainers think about emitting "close".

For your reference, light-my-request, the HTTP mocking API (also no server) used in the popular library fastify, also emit the close event: https://github.com/fastify/light-my-request/blob/master/lib/response.js#L98-L107

There is also a comment here that mention: https://github.com/fastify/light-my-request/blob/46781cc6cf826d3c2fc0494b07a02395125a7fbf/lib/response.js#L92-L93

  // We need to emit 'close' otherwise stream.finished() would
  // not pick it up on Node v16

Nearby them, however, there is a finish event, which seems to be a good candidate too since this event is always fired afaik:

https://github.com/fastify/light-my-request/blob/master/lib/response.js#L90

Response.prototype.end = function (data, encoding, callback) {
  if (data) {
    this.write(data, encoding)
  }

  http.ServerResponse.prototype.end.call(this, callback)

  this.emit('finish')

  // We need to emit 'close' otherwise stream.finished() would
  // not pick it up on Node v16

  this.destroy()
}

[hoangvvo]

Looks like node-mocks-http support finish event too: https://github.com/howardabrams/node-mocks-http/blob/master/lib/mockResponse.js#L472. This one might be better altogether.

I have switched the code over to make use of the finish event.

[jakeorr]

Nice, yeah finish works for me too. Seems like a good change! I'm happy with how this is looking.

[hoangvvo]

Thanks for the review @jakeorr. I really appreciate it! Will create a release soon after this.