Return reply from fastify handler

[mwoenker]

See https://www.fastify.io/docs/latest/Guides/Migration-Guide-V4/#need-to-return-reply-to-signal-a-fork-of-the-promise-chain

This fixes returning a Stream as the body of the response from an api function.

[jtoar]

Hey @mwoenker thanks for your PR and patience! I thought I saw this brought up somewhere else, but I can't remember whether it was an issue, or on the forums or Discord. (I tried searching each briefly but couldn't find anything.) This change seems simple enough and adds up based on the doc you linked to. @dthyresson any thoughts?

[jtoar]

Do we need to make the change in handlerCallback too?

redwood/packages/api-server/src/requestHandlers/awsLambdaFastify.ts

Lines 69 to 78 in 8c6bc8a

	const handlerCallback =
	(reply: FastifyReply) =>
	(error: Error, lambdaResult: APIGatewayProxyResult) => {
	if (error) {
	fastifyResponseForLambdaError(req, reply, error)
	return
	}
	fastifyResponseForLambdaResult(reply, lambdaResult)
	}

[dthyresson]

Thanks @mwoenker

Is there a way to test this behavior in packages/api-server/src/__tests__/requestHandlers/awsLambdaFastify.test.ts?

Something like:

  test('requestHandler replies with simple body', async () => {
    jest.spyOn(mockedReply, 'send')
    jest.spyOn(mockedReply, 'status')

    const handler = async (req, mockedReply) => {
      mockedReply = { body: { foo: 'bar' } }
      return mockedReply
    }

    await requestHandler(request, mockedReply, handler)

    expect(mockedReply.send).toHaveBeenCalledWith({ foo: 'bar' })
    expect(mockedReply.status).toHaveBeenCalledWith(200)
  })

But for a stream?

[mwoenker]

@dthyresson I've pushed a test. It tests to see if the stream is passed to send(), but also whether the reply is returned from requestHandler which is the thing that will actually change as a result of this patch. Maybe the other tests should check that as well?

[replay-io]

10 replays were recorded for 11178a3.

0 Failed

10 Passed

• requireAuth graphql checks
• useAuth hook, auth redirects checks
• RBAC: Admin user should be able to delete contacts
• RBAC: Should not be able to delete contact as non-admin user
• Smoke test with dev server
• Smoke test with rw serve
• Loads Cell mocks when Cell is nested in another story
• Loads Cell Stories
• Loads MDX Stories
• Mocks current user, and updates UI while dev server is running

View test run on Replay ↗︎

[dthyresson]

@dac09 I think this is the issue we fixed this morning #6765. I wonder if we should add his test as well?

[jtoar]

@mwoenker thanks for your work here! @dthyresson confirming we don't actually want to add the test here?