Replies as Promises

[mgcrea]

Context

Writing jest mocks for a got client (eg. got.extend()), was looking into dynamically loaded fixtures as replies. Found out that promises and async/await functions where not handled properly.

Alternatives

Force the code to be synchronous (doable in my case with the fs package).

Has the feature been requested before?

Did not found it

If the feature request is accepted, would you be willing to submit a PR?

Yes, added unit tests already, but had a look at the code and did not found it trivial to add it. Found that there was no async handling (only setting this.body) and did not really found out where the Promise.resolve() could fit, any guidance/ideas would be great.

Example

A few tests:

test('reply can take a promise', async t => {
  const scope = nock('http://example.com')
    .get('/')
    .reply(200, async (path, requestBody, callback) => 'Hello World!')

  const response = await got('http://example.com/', {
    encoding: null,
  })

  scope.done()
  t.type(response.body, Buffer)
  t.equal(response.body.toString('utf8'), 'Hello World!')
})

test('reply takes a promise for status code', async t => {
  const expectedStatusCode = 202
  const responseBody = 'Hello, world!'
  const headers = {
    'X-Custom-Header': 'abcdef',
  }

  const scope = nock('http://example.com')
    .get('/')
    .reply(async (path, requestBody, cb) => {
      await new Promise((resolve, reject) => {
        setTimeout(resolve, 1)
      })
      return [expectedStatusCode, responseBody, headers]
    })

  const response = await got('http://example.com/')

  t.equal(response.statusCode, expectedStatusCode, 'sends status code')
  t.deepEqual(response.headers, headers, 'sends headers')
  t.equal(response.body, responseBody, 'sends request body')
  scope.done()
})

[paulmelnikow]

Thanks for this!

I might be misunderstanding, though it is possible to specify replies using an error-first callback: https://github.com/nock/nock#specifying-replies

I think support could be added there for promise-returning functions, without making a breaking change. The code could check the return value, and if it's a promise, await it, and if not, wait for the callback to be called.

Does that make sense? Are you interested in taking it on? A contribution would be welcome!

[mastermatt]

I believe the correct way to do this would be to replace this line with the following.

nock/lib/request_overrider.js

Line 322 in 900d96d

continueWithResponseBody(null, replyResponseBody)

      Promise.resolve(replyResponseBody)
        .then(body => continueWithResponseBody(null, body))
        .catch(err => continueWithResponseBody(err));

This would allow the callback that accepts two args to return either data for the body directly (current behavior) or a Promise that resolves to the data.

A similar change would also be required in the next block that deals with fullReplyFunction.

This will mean adding an extra tick to the end method on the request overridder, and I can't tell if that will have negative consequences without testing.

[paulmelnikow]

Hmmm, I wonder if we can flip around the implementations the other way, and base them around async. We could util.promisify the callback.

I feel like that chain of functions would become easier to follow using async.

[mastermatt]

This has been resolved with #1596 and the new feature is in Beta. Publication still pending.
@mgcrea would you mind giving it a whirl?