Refactor AWS Lambda streaming support and enhance converters - #37
Refactor AWS Lambda streaming support and enhance converters#37conico974 wants to merge 5 commits into
Conversation
commit: |
There was a problem hiding this comment.
Pull request overview
This PR refactors the converter/wrapper contract to better support AWS Lambda streaming by introducing a ConverterOutput structure (direct vs stream), removing chunk-retention behavior, and updating wrappers/converters to either stream responses or buffer streams into platform-specific return values. It also adds/updates unit tests to cover the new streaming converter and the updated converter behaviors.
Changes:
- Introduces
ConverterOutputand updates theConverter.convertTo(event, context)API to return either a stream creator (optionally with a deferred platformoutput) or a direct finalizer. - Refactors wrappers (AWS Lambda, AWS Lambda streaming/compressed, Node, Cloudflare) to initialize converter output before invoking the handler and to handle direct vs streamed results consistently.
- Adds a buffered stream creator utility for AWS API Gateway/CloudFront converters and adds/updates unit tests for the new converter behaviors (including the new
aws-streamingconverter).
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tests-unit/tests/core/routing/util.test.ts | Updates routing util tests to reflect removal of buffered body exposure. |
| packages/tests-unit/tests/converters/sqs-revalidate.test.ts | Adds unit coverage for direct-output behavior of the SQS revalidate converter. |
| packages/tests-unit/tests/converters/aws-streaming.test.ts | Adds coverage for Lambda streaming prelude + streamed body behavior. |
| packages/tests-unit/tests/converters/aws-cloudfront.test.ts | Refactors tests to validate buffered streaming output path for CloudFront converter. |
| packages/tests-unit/tests/converters/aws-apigw-v2.test.ts | Refactors tests to validate buffered streaming output path for API Gateway v2 converter. |
| packages/tests-unit/tests/converters/aws-apigw-v1.test.ts | Refactors tests to validate buffered streaming output path for API Gateway v1 converter. |
| packages/core/src/types/overrides.ts | Adds ConverterOutput and updates Converter.convertTo signature/semantics. |
| packages/core/src/types/open-next.ts | Makes InternalResult.body optional, removes retainChunks, adds aws-streaming converter option. |
| packages/core/src/plugins/resolve.ts | Adds defaulting behavior to select aws-streaming converter for aws-lambda-streaming wrapper. |
| packages/core/src/overrides/wrappers/node.ts | Updates node wrapper to use new converter output model. |
| packages/core/src/overrides/wrappers/express-dev.ts | Updates dev wrapper to use new converter output model for image/all routes. |
| packages/core/src/overrides/wrappers/dummy.ts | Updates dummy wrapper to use new converter output model. |
| packages/core/src/overrides/wrappers/cloudflare-node.ts | Refactors Cloudflare-node wrapper to rely on converter stream output and return output. |
| packages/core/src/overrides/wrappers/cloudflare-edge.ts | Refactors Cloudflare-edge wrapper to initialize converter output early and support direct exceptional results. |
| packages/core/src/overrides/converters/node.ts | Moves Node converter to provide a StreamCreator via convertTo instead of returning an InternalResult mapping. |
| packages/core/src/overrides/converters/edge.ts | Refactors Edge converter to return streaming output + direct exceptional handler for middleware results. |
| packages/core/src/overrides/converters/dummy.ts | Updates dummy converter to return direct ConverterOutput. |
| packages/core/src/http/openNextResponse.ts | Removes chunk retention and related helpers from OpenNextNodeResponse. |
| packages/core/src/core/routing/util.ts | Updates convertRes to stop synthesizing a body from retained chunks. |
| packages/core/src/core/requestHandler.ts | Adjusts request handler to handle optional bodies and updated convertRes output. |
| packages/core/src/build/validateConfig.ts | Updates wrapper/converter compatibility matrix and defaults for Lambda streaming. |
| packages/core/src/build/generateOutput.ts | Updates default converter selection when wrapper is aws-lambda-streaming. |
| packages/aws/src/overrides/wrappers/aws-lambda.ts | Refactors AWS Lambda wrapper to use new converter output model (direct vs stream). |
| packages/aws/src/overrides/wrappers/aws-lambda-streaming.ts | Refactors Lambda streaming wrapper to delegate streaming response construction to converter. |
| packages/aws/src/overrides/wrappers/aws-lambda-compressed.ts | Refactors compressed wrapper to apply compression via StreamCreator composition. |
| packages/aws/src/overrides/converters/sqs-revalidate.ts | Updates SQS revalidate converter to return direct output with data finalizer. |
| packages/aws/src/overrides/converters/response-stream.ts | Introduces a buffered StreamCreator helper to build platform outputs from streamed writes. |
| packages/aws/src/overrides/converters/aws-streaming.ts | Adds new aws-streaming converter for Lambda HTTP integration streaming prelude + body streaming. |
| packages/aws/src/overrides/converters/aws-cloudfront.ts | Refactors CloudFront converter to buffered streaming output with a middleware direct handler. |
| packages/aws/src/overrides/converters/aws-apigw-v2.ts | Refactors API Gateway v2 converter to buffered streaming output. |
| packages/aws/src/overrides/converters/aws-apigw-v1.ts | Refactors API Gateway v1 converter to buffered streaming output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Updated aws-lambda-streaming wrapper to utilize new converter structure. - Introduced aws-streaming converter for handling streaming responses in AWS Lambda. - Enhanced existing converters to support direct and streaming output types. - Modified core types to accommodate new converter output structure. - Updated validation logic to ensure compatibility with new streaming converter. - Added tests for aws-streaming converter and SQS revalidate functionality. - Refactored express-dev and cloudflare wrappers to align with new converter outputs. - Improved response handling in node and cloudflare-node wrappers.
… improved memory management
113b5a0 to
2e7d4c3
Compare
# Conflicts: # packages/aws/src/overrides/converters/aws-apigw-v1.ts # packages/aws/src/overrides/converters/aws-apigw-v2.ts # packages/aws/src/overrides/converters/aws-cloudfront.ts # packages/core/src/build/validateConfig.ts # packages/core/src/overrides/converters/edge.ts # packages/core/src/overrides/converters/node.ts # packages/core/src/plugins/resolve.ts
|
The current main branch was merge into here and AI helped with the last commit. Below is a report of the changes: AWS Production Code
Core Build And Resolution
Core Streaming Lifecycle
Core Tests
Converter And Wrapper Tests
Review Comments
|
conico974
left a comment
There was a problem hiding this comment.
I think I didn't make the point of this PR clear enough, it's basically this https://github.com/orgs/opennextjs/projects/3?pane=issue&itemId=149553272
Basically every function that returns an InternalResult should stream, and the rest should depend on the converter and wrapper. I think we might be missing something here
There was a problem hiding this comment.
This file should probably go somewhere else than in wrappers since it is also used by some converters
There was a problem hiding this comment.
It's probably ok for this PR, but we need to redo this file entirely, generateOutput should either be in aws or written in a completely overridable way, it doesn't make sense to have aws constants in core
| } | ||
| } | ||
| routingResult.body = bodyToReturn; | ||
| routingResult.body = undefined; |
There was a problem hiding this comment.
We are never returning routingResult body right ? If that's the case it should not be a property outside of here.
| waitUntil: ctx.waitUntil.bind(ctx), | ||
| }) | ||
| ); | ||
| const handlerPromise = handler(internalEvent, { |
There was a problem hiding this comment.
It's probably a typing issue here, and for all the other wrappers, but the idea is that if output.type is not direct it means we need to stream.
The whole point is that everything should stream except for some very specific one (like the queue handler, in which case you need to use the appropriate converter)
| | "server" | ||
| | "middleware" | ||
| | "edge" | ||
| | "global" |
There was a problem hiding this comment.
What is this global thing here ? Placement global was only ever used to signify that it was supposed to use lambda@edge.
My gut would be to move this in aws not in core, aws can have it's own open-next.config.ts types or helper like cloudflare has
Refactor AWS Lambda streaming support by introducing a new converter structure and enhancing existing converters for better handling of direct and streaming output types. Improve memory management by removing the retainChunks option and updating response handling. Add tests for the new aws-streaming converter and ensure compatibility with existing functionality.