Async Reply functions (always emit errors)

[mastermatt]

This is a POC for #1557 and is offered as an Option B to #1572.

The difference between this change and #1572 is that this takes a hard line on how to handle errors in reply functions. It always emits an error (105fda4).

I personally believe this is the right design long term but it does introduce a breaking change.

Using the example from the docs:

const scope = nock('http://www.google.com')
  .post('/echo')
  .reply(201, (uri, requestBody, cb) => {
    fs.readFile('cat-poems.txt', cb) // Error-first callback
  })

Currently, if readFile errs, the request will succeed with a response that has a 500 status code.
To achieve the same affect, the code would have to be rewritten by the consumer to:

const scope = nock('http://www.google.com')
  .post('/echo')
  .reply((uri, requestBody, cb) => {
    fs.readFile('cat-poems.txt', (err, contents) => {
      if (err) cb([500, err.stack])
      else cb([201, contents])
    })
  })

However, I would argue that this is not the standard use case nor the most intuitive handling of a test.

[paulmelnikow]

Hey Matt, is your feeling still that this is a better way to go? If it is I will aim to dig in and leave some comments in the next day or so.

[mastermatt]

This version gets my vote unless we deem the breaking changes undesirable.

[paulmelnikow]

It would make a better assertion if you passed something more specific in, probably both for the path and the body.

[paulmelnikow]

You could also consider checking if it is an AsyncFunction (and in that case not promisifying, regardless of the number of arguments):

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AsyncFunction

It ensures that an async reply function is awaited, even if the arguments have gotten messed up.

[mastermatt]

I have reservations on this. An async function and a function returning a promise should be treated the same in my book, but there is no way to sniff for the later.

With some quick testing, it looks like even if an async fn gets wrapped in promisify it will resolve the whole chain as desired.

[paulmelnikow]

I agree with the principle that an async function and a promise should be treated the same.

A conflicting principle (in this case) is that the number of arguments to a promise-returning function should not affect its behavior. I see why we did that here, and when we lived in the world of callbacks, why it would be necessary. Though going forward ideally we wouldn't care about the number of arguments to the function. My goal is to avoid surprises when someone makes a mistake.

I don't think we need to belabor this (and it's not so important in the scheme of things) – just wanted to convey where I was coming from.

[mastermatt]

@paulmelnikow based on other convos we've had in other PRs; would the preference be to change the calls of t.rejects to use assert-rejects?

[paulmelnikow]

If we're going to change to a different test runner I think we may as well keep using assertRejects though I don't much care either way.

[nockbot]

🎉 This PR is included in version 11.0.0-beta.31 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nockbot]

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀