fix: preserve multi-value response headers in dev#646
Conversation
Functions served through the Vite plugin were rebuilding Web Responses from lambda results using only `headers`, but these v2 functions were returning their response metadata in `multiValueHeaders`. Merge both sets when reconstructing the Response and add regression tests for plain and streamed responses. Made-with: Cursor
📝 WalkthroughWalkthroughAdds two Vitest cases under "Functions with the v2 API syntax" that verify header propagation from v2 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/functions/dev/src/main.test.ts (1)
153-194: Recommended: add fixture cleanup in both new tests.These tests create temporary fixtures but don’t call
fixture.destroy(). Wrapping each test body intry/finallywill reduce leakage/flakiness when assertions fail early.♻️ Suggested pattern
const fixture = new Fixture().withFile('netlify/functions/headers.mjs', source) - const directory = await fixture.create() - const destPath = join(directory, 'functions-serve') - const functions = new FunctionsHandler({ ... }) - - const req = new Request('https://site.netlify/headers') - const match = await functions.match(req, destPath) - expect(match).not.toBeUndefined() - - const res = await match!.handle(req) - expect(res.status).toBe(401) - ... + const directory = await fixture.create() + try { + const destPath = join(directory, 'functions-serve') + const functions = new FunctionsHandler({ ... }) + + const req = new Request('https://site.netlify/headers') + const match = await functions.match(req, destPath) + expect(match).not.toBeUndefined() + + const res = await match!.handle(req) + expect(res.status).toBe(401) + ... + } finally { + await fixture.destroy() + }Also applies to: 196-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/functions/dev/src/main.test.ts` around lines 153 - 194, The new tests create temporary fixtures via Fixture().withFile(...) and fixture.create() but never call fixture.destroy(), which can leak temp files if assertions fail; wrap each test body (both the 'Preserves response headers for v2 functions' test and the similar test at lines 196-243) in a try/finally and call fixture.destroy() in the finally block (i.e., ensure fixture.destroy() is invoked after using fixture, matching the lifecycle created by Fixture().withFile and fixture.create()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/functions/dev/src/main.test.ts`:
- Around line 153-194: The new tests create temporary fixtures via
Fixture().withFile(...) and fixture.create() but never call fixture.destroy(),
which can leak temp files if assertions fail; wrap each test body (both the
'Preserves response headers for v2 functions' test and the similar test at lines
196-243) in a try/finally and call fixture.destroy() in the finally block (i.e.,
ensure fixture.destroy() is invoked after using fixture, matching the lifecycle
created by Fixture().withFile and fixture.create()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8882c20e-d979-44d5-8771-ec2da31cb4e0
📒 Files selected for processing (2)
packages/functions/dev/src/main.test.tspackages/functions/dev/src/runtimes/nodejs/lambda.ts
|
This was found while developing a Netlify Function that serves an MCP server using Streamable HTTP responses and testing it with Claude Code. Everything worked fine when running the edge function using After investigation, we found that the local Netlify function runtime path used by In our real failure case, the important HTTP headers that went missing on the Vite-plugin path included:
And for the streamed MCP case specifically, losing |
|
Fixes #647 |
|
The automated PR review has a "nit": However, I'm actively ignoring that. None of the other tests in main.test.ts worry about that, and my new test follow the patterns already established in the file. |
Drop the now-unused webHeadersFromHeadersObject helper after switching response reconstruction to operate on the full lambda response shape. Made-with: Cursor
|
@aguynamedben this is great, thank you! The failures in the CI are due to some weirdness in our current setup. Let me get that sorted and I'll get this shipped. |
Summary
headersandmultiValueHeaderswhen@netlify/functions-devreconstructs a WebResponsefrom the lambda-style resultThis fixes a local dev mismatch where Functions served through
@netlify/vite-pluginreturned the correct body but silently lost response headers such ascontent-type,cache-control,www-authenticate, CORS headers, and custom headers. The same Functions served throughnetlify devpreserved those headers.The root cause was that
webResponseFromLambdaResponse(...)only readlambdaResponse.headers, while the real runtime result for these functions was carrying the response metadata inmultiValueHeaders.Example
A standalone repro app that compares
@netlify/vite-pluginagainstnetlify devshowed this before the fix:After this change, the same repro goes green for:
/api/headers/api/auth-challenge/api/sseTest plan
npm run test --workspace=@netlify/functions-dev@netlify/vite-pluginvsnetlify devMade with Cursor