fix JS and TS executors to await Promise in context execution #246
Conversation
🦋 Changeset detectedLatest commit: e4aa98a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Clarifies that JS/TS code returning Promises is automatically awaited, ensuring users understand they receive resolved values in results. Related to cloudflare/sandbox-sdk#246
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
The Promise resolution logic is correct and consistently applied to both executors. The thenable check properly detects Promises and the await is correctly placed inside the try-catch block.
Two issues need addressing:
-
Missing test for rejected Promises - The E2E tests cover 6 scenarios for resolved Promises but zero for rejected ones. Need tests for
Promise.reject()and async functions that throw to verify error handling works correctly. -
TypeScript standards violation - Using
(result as any).thenviolates CLAUDE.md's "never use any" rule. Should define a properThenableinterface with a type guard function instead.
See inline comments for specific fixes.
Verdict: Needs fixes before merge
packages/sandbox-container/src/runtime/executors/javascript/node_executor.ts
Outdated
Show resolved
Hide resolved
packages/sandbox-container/src/runtime/executors/typescript/ts_executor.ts
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add documentation for automatic Promise resolution in JavaScript and TypeScript code contexts. This clarifies the behavior fixed in PR #246 where async functions and Promise.resolve() now correctly return their resolved values instead of empty objects. Changes: - Add async code execution examples to API reference - Add async error handling examples - Add practical async/await guide section - Document that Promises are automatically awaited Related: cloudflare/sandbox-sdk#246
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-246-97c9b17Version: You can use this Docker image with the preview package from this PR. |
Fixes the JavaScript and TypeScript executors to properly await Promises during context execution.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Adds documentation for automatic Promise resolution in JavaScript and TypeScript contexts. The Code Interpreter now properly awaits Promise results, allowing async/await patterns to work correctly. Includes examples for: - Async IIFE patterns - Direct Promise.resolve usage - Nested async operations - Async error handling with Promise.reject References cloudflare/sandbox-sdk#246
| // Async/Await Promise Handling (Issue #206) | ||
| // ============================================================================ | ||
|
|
||
| test('should resolve async IIFE and return the value', async () => { |
There was a problem hiding this comment.
Just to validate - all the tests are wrapped in an IIFE. Would this change work if the user passed code not wrapped in an IIFE? If not, is the answer then to wrap the underlying implementation in an async IIFE?
There was a problem hiding this comment.
@ghostwriternr to support top-level await we'd need to wrap the code in an async IIFE, which adds complexity (need to handle "last expression" capture for multi-statement code) and changes. For this PR, I'd suggest keeping as is since it solves the issue. We can open a separate issue for top-level await? or should we just do it properly here?
Adds practical examples of async JavaScript/TypeScript execution to the how-to guide, demonstrating data processing workflows with Promises. References cloudflare/sandbox-sdk#246
|
going to start fresh to do the top level await, not just this fix |
2 participants
top level await, i played a lot with just wrapping all code and tbh, i wasn't happy with it, felt hacky
so now we parse code with acord, then we check if last AST node is an
ExpressionStatement, if the answer is yes: wrap code withreturnon last expression, if no: wrap code without return (returnsundefined)