Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
src/cmd/transpile.jsOops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by src/cmd/opt.jsOops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by test/async.jsOops! Something went wrong! :( ESLint: 9.19.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by
WalkthroughThis pull request introduces comprehensive support for asynchronous WebAssembly component transpilation across multiple platforms and environments. The changes span several key areas: adding async mode configurations, enhancing transpilation options, updating type generation, and implementing new testing strategies. The modifications enable developers to specify async behaviors using JavaScript Promise Integration (JSPI) and Asyncify, with flexible configuration options for imports and exports. Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant CLI as JCO CLI
participant Transpiler as Component Transpiler
participant Generator as Code Generator
participant Runtime as JavaScript Runtime
Dev->>CLI: Specify async mode (jspi/asyncify)
CLI->>Transpiler: Configure async options
Transpiler->>Generator: Generate async-aware bindings
Generator->>Runtime: Produce async-compatible JavaScript
Runtime->>Runtime: Execute async WebAssembly component
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (27)
test/helpers.js (6)
1-22: Consider reorganizing imports for readability and modularization.
The list of Node.js built-in modules is large, and grouping them by type (e.g., core modules vs external dependencies) could increase maintainability.Proposed minor improvement:
import { version, env, argv, execArgv } from "node:process"; +// Node.js core modules import { createServer as createNetServer } from "node:net"; import { createServer as createHttpServer } from "node:http"; import { basename, join, isAbsolute, resolve, normalize, sep, relative, dirname, extname, } from "node:path"; import { cp, mkdtemp, writeFile, stat, mkdir, readFile, } from "node:fs/promises"; +// External dependencies import mime from "mime"; import { pathToFileURL } from "url"; import { transpile } from "../src/api.js"; import { componentize } from "../src/cmd/componentize.js";
36-55: Ensure consistent logging approach.
Thelogfunction here conditionally prints debugging information, but other debugging or error messages in the file appear to directly useconsole.log. Consider using this helper consistently to handle all debug statements.+// Example: Possibly replace direct console.log calls with `log` - console.log("some debug message") + log("some debug message")
Line range hint
57-75: Enhance error handling in theexecutility.
Currently, this function rejects with the same error message for both stderr and stdout. Distinguishing between build or runtime errors could make debugging easier.cp.on("exit", (code) => code === 0 ? resolve() - : reject(new Error((stderr || stdout).toString())) + : reject( + new Error( + `Failure executing '${cmd}':\n` + + `Exit code: ${code}\n` + + `stderr: ${stderr}\n stdout:${stdout}` + ) + ) );
117-120: Clarify error message for conflicting component definitions.
The exceptionBoth component.path and component.build should not be specified...is correct, but adding context for the user can improve debugging.- throw new Error( - "Both component.path and component.build should not be specified at the same time", - ); + throw new Error( + "Both 'component.path' and 'component.build' are set in 'setupAsyncTest': Only one can be provided." + );
218-227: Consider adding timeout or cancellation logic in the transpilation process.
The code reads and writes multiple files, which can be time-consuming. Handling potential slow operations or user cancellations can improve user experience.Would you like utility code injected offering a timeout or cancellation token?
473-547: Return more detailed status codes when the server fails to find a file.
Currently, only404or500is returned. Clarifying the reason or providing custom status codes could benefit debugging.const returnError = (e) => { log(`[webserver] failed to find file [${fileURL}]`); - res.writeHead(404); + if (e.code === "ENOENT") { + res.writeHead(404, "File Not Found"); + } else { + res.writeHead(500, "Internal Server Error"); + } res.end(e.message); };crates/js-component-bindgen/src/intrinsics.rs (1)
153-184: Synchronous asyncify instantiation.
The approach is essentially the same, but synchronous. Consider clarifying in docs that this still sets upasyncifyModules. This can be confusing if “sync” usage is truly synchronous in user code.- * Synchronous version of the instantiation process for asyncify + * Synchronous version uses the same memory management approach but + * does not return a Promise for instantiation. The `asyncifyModules` + * array is still shared.crates/js-component-bindgen/src/ts_bindgen.rs (2)
35-36: Introduceasync_importsandasync_exports.
Storing these sets inTsBindgenallows flexible referencing of asynchronous functions.Consider documenting how these sets are populated, to help future maintainers.
/// ... +/// `async_imports` & `async_exports` store names of functions that must be treated async. +/// They’re initialized based on user-supplied options or a default approach. struct TsBindgen { src: Source, // ... async_imports: HashSet<String>, async_exports: HashSet<String>,
484-494: Check function name against async sets.
This logic correctly checks for the function name, world name, and possible versioned references.You might factor out the name-check code into a helper to improve testability and maintainability.
crates/js-component-bindgen/src/transpile_bindgen.rs (3)
138-142: Tracking async exports with a boolean is workable.
You might consider using an enum or dedicated struct for clarity, but the boolean flag is fine for now.
290-297: Providing defaultgetCoreModuleis well-structured.
Consider logging or error handling iffetchis unavailable in some Node environments.
1277-1295: Dynamically generating trampolines is well-organized.
Be aware of your environment’s support forWebAssembly.Suspending. Providing a fallback might be desirable.crates/js-component-bindgen-component/wit/js-component-bindgen.wit (1)
61-72: Consider grouping related fields together.The
guestfield seems out of place betweenno-namespaced-exportsandmulti-memory. Consider grouping it with other related configuration fields for better organization.test/async.js (1)
54-69: Consider adding negative test cases.While the basic transpilation test is good, consider adding test cases for invalid configurations or error scenarios.
src/cmd/opt.js (1)
142-145: Enhance error handling in wasmOpt function.Consider adding more specific error handling for different types of wasm-opt failures, not just the 'BasicBlock requested' case.
test/async.browser.js (2)
110-111: Consider headless mode and error handling for browser launch.The browser launch should include error handling and preferably use headless mode for CI environments.
- const browser = await puppeteer.launch(); + const browser = await puppeteer.launch({ + headless: 'new', + }).catch(error => { + console.error('Failed to launch browser:', error); + throw error; + });
44-48: Enhance cleanup error handling.The cleanup error handling could be improved by logging errors and ensuring all resources are properly released.
suiteTeardown(async function () { try { await rm(tmpDir, { recursive: true }); - } catch {} + } catch (error) { + console.error('Failed to clean up temporary directory:', error); + } }); teardown(async function () { try { await rm(outDir, { recursive: true }); await rm(outFile); - } catch {} + } catch (error) { + console.error('Failed to clean up test artifacts:', error); + } });Also applies to: 50-55
src/jco.js (1)
84-88: Consider grouping related async options.The async options in the types command could be grouped together using Option.Group for better organization.
+ .addOption(new Option.Group() + .addOption(new Option('--async-mode [mode]', 'EXPERIMENTAL: use async imports and exports').choices(['sync', 'jspi', 'asyncify']).preset('sync')) + .addOption(new Option('--default-async-imports', 'EXPERIMENTAL: default async component imports from WASI interfaces')) + .addOption(new Option('--default-async-exports', 'EXPERIMENTAL: default async component exports from WASI interfaces')) + .addOption(new Option('--async-imports <imports...>', 'EXPERIMENTAL: async component imports (examples: "wasi:io/poll@0.2.0#poll", "wasi:io/poll#[method]pollable.block")')) + .addOption(new Option('--async-exports <exports...>', 'EXPERIMENTAL: async component exports (examples: "wasi:cli/run@#run", "handle")')) + .name('Async Options (EXPERIMENTAL)') + ) - .addOption(new Option('--async-mode [mode]', 'EXPERIMENTAL: use async imports and exports').choices(['sync', 'jspi', 'asyncify']).preset('sync')) - .option('--default-async-imports', 'EXPERIMENTAL: default async component imports from WASI interfaces') - .option('--default-async-exports', 'EXPERIMENTAL: default async component exports from WASI interfaces') - .option('--async-imports <imports...>', 'EXPERIMENTAL: async component imports (examples: "wasi:io/poll@0.2.0#poll", "wasi:io/poll#[method]pollable.block")') - .option('--async-exports <exports...>', 'EXPERIMENTAL: async component exports (examples: "wasi:cli/run@#run", "handle")')test/cli.js (3)
642-669: LGTM! Consider adding cache size limits.The cache implementation is well-structured and properly documented. However, since it stores outputs in memory without limits, consider adding a maximum cache size or an LRU mechanism to prevent potential memory issues with large components.
156-190: Consider reducing test duplication.The Asyncify test duplicates much of the JSPI test logic. Consider extracting the common test logic into a shared helper function that takes the async mode as a parameter.
+function testAsyncMode(mode, { requiresSuspending = false } = {}) { + const runTest = async () => { + const name = "async_call"; + const { stderr } = await exec( + jcoPath, + "transpile", + `test/fixtures/components/${name}.component.wasm`, + `--name=${name}`, + "--valid-lifting-optimization", + "--tla-compat", + "--instantiation=async", + "--base64-cutoff=0", + `--async-mode=${mode}`, + "--async-imports=something:test/test-interface#call-async", + "--async-exports=run-async", + "-o", + outDir, + ); + strictEqual(stderr, ""); + await writeFile( + `${outDir}/package.json`, + JSON.stringify({ type: "module" }), + ); + const m = await import(`${pathToFileURL(outDir)}/${name}.js`); + const inst = await m.instantiate(undefined, { + "something:test/test-interface": { + callAsync: async () => "called async", + callSync: () => "called sync", + }, + }); + strictEqual(inst.runSync instanceof AsyncFunction, false); + strictEqual(inst.runAsync instanceof AsyncFunction, true); + + strictEqual(inst.runSync(), "called sync"); + strictEqual(await inst.runAsync(), "called async"); + }; + + if (requiresSuspending && typeof WebAssembly.Suspending !== "function") { + return; + } + + test(`Transpile with Async Mode for ${mode}`, runTest); +} + +testAsyncMode("jspi", { requiresSuspending: true }); +testAsyncMode("asyncify");
121-135: Enhance error handling for exec calls.Consider implementing structured error handling for exec calls to capture both stderr and stdout:
- Create a helper function that checks both stdout and stderr
- Implement proper error propagation
- Add error context for debugging
Example implementation:
async function execWithErrorHandling(command, ...args) { const { stdout, stderr } = await exec(command, ...args); if (stderr) { throw new Error(`Command failed with stderr: ${stderr}\nCommand: ${command} ${args.join(' ')}`); } return { stdout, stderr }; }Also applies to: 158-172
crates/js-component-bindgen/src/function_bindgen.rs (2)
1052-1058: Consider using template literals for better readability.Replace string concatenation with template literals for better readability and maintenance:
-let maybe_async_await = if self.is_async { "await " } else { "" }; -uwriteln!( - self.src, - "{maybe_async_await}{}({});", - self.callee, - operands.join(", ") -); +let prefix = if self.is_async { "await " } else { "" }; +uwriteln!( + self.src, + "{}{}({});", + prefix, + self.callee, + operands.join(", ") +);
1076-1089: Consider extracting common async handling logic.The async handling pattern is duplicated between CallWasm and CallInterface. Consider extracting it into a helper method:
impl FunctionBindgen<'_> { fn format_async_call(&self, call: String) -> String { if self.is_async { format!("await {}", call) } else { call } } }test/fixtures/browser/test-pages/something__test.async.html (3)
2-24: Improve comment formatting for better readability.Consider using a more structured format for the interface documentation:
-<!-- - -This page tests a component with the following WIT interface: - -``` +<!-- + This page tests a component with the following WIT interface: + + ```wit
82-84: Define constants for timing values.Magic numbers (50ms interval) should be defined as named constants for better maintainability.
+const TIMER_INTERVAL_MS = 50; + const wake = setInterval(() => { count += 1; -}, 50); +}, TIMER_INTERVAL_MS);
90-92: Define constant for expected count and improve error message.The magic number 20 should be a named constant, and the error message could be more descriptive.
+const EXPECTED_MIN_COUNT = 20; // Minimum expected count after 2000ms (40 intervals of 50ms) + -if (count <= 20) { - err(`ERROR setTimeout did not run unimpeded, only completed [${count}] 50ms increments (expected ~20)`); +if (count <= EXPECTED_MIN_COUNT) { + err(`ERROR: Async operation blocked the main thread. Only completed ${count} intervals of ${TIMER_INTERVAL_MS}ms (expected >${EXPECTED_MIN_COUNT})`);docs/src/transpiling.md (1)
56-58: Fix punctuation and add usage examples for experimental features.
- Fix the punctuation issues:
-* `--async-mode [mode]`: EXPERIMENTAL: For the component imports... -* `--async-imports <imports...>`: EXPERIMENTAL: Specify the component... -* `--async-exports <exports...>`: EXPERIMENTAL: Specify the component... +* `--async-mode [mode]`: EXPERIMENTAL - For the component imports... +* `--async-imports <imports...>`: EXPERIMENTAL - Specify the component... +* `--async-exports <exports...>`: EXPERIMENTAL - Specify the component...
- Consider adding usage examples for these experimental features to make them more accessible to users.
Would you like me to generate example usage snippets for these experimental features?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript *--async-mode [mode]: EXPERIMENTAL: For the component imports...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). *--async-imports <imports...>`: EXPERIMENTAL: Specify the component imp...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. *--async-exports <exports...>`: EXPERIMENTAL: Specify the component exp...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/components/async_call.component.wasmis excluded by!**/*.wasm
📒 Files selected for processing (21)
crates/js-component-bindgen-component/src/lib.rs(3 hunks)crates/js-component-bindgen-component/wit/js-component-bindgen.wit(2 hunks)crates/js-component-bindgen/src/core.rs(0 hunks)crates/js-component-bindgen/src/function_bindgen.rs(3 hunks)crates/js-component-bindgen/src/intrinsics.rs(8 hunks)crates/js-component-bindgen/src/lib.rs(1 hunks)crates/js-component-bindgen/src/transpile_bindgen.rs(19 hunks)crates/js-component-bindgen/src/ts_bindgen.rs(16 hunks)docs/src/transpiling.md(1 hunks)package.json(1 hunks)src/cmd/opt.js(4 hunks)src/cmd/transpile.js(8 hunks)src/jco.js(2 hunks)test/async.browser.js(1 hunks)test/async.js(1 hunks)test/cli.js(33 hunks)test/fixtures/browser/test-pages/something__test.async.html(1 hunks)test/helpers.js(3 hunks)test/test.js(2 hunks)xtask/src/build/jco.rs(1 hunks)xtask/src/generate/wasi_types.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/js-component-bindgen/src/core.rs
🧰 Additional context used
🪛 LanguageTool
docs/src/transpiling.md
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript * --async-mode [mode]: EXPERIMENTAL: For the component imports...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). * --async-imports <imports...>`: EXPERIMENTAL: Specify the component imp...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. * --async-exports <exports...>`: EXPERIMENTAL: Specify the component exp...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome (1.9.4)
test/helpers.js
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Jco Build
🔇 Additional comments (73)
test/helpers.js (1)
387-459: Validate browser environment prior to testing.
The functionloadTestPagerelies on Puppeteer. If Puppeteer is missing or incompatible, the test might appear to stall. Consider verifying that thebrowserobject is a valid Puppeteer instance.crates/js-component-bindgen/src/intrinsics.rs (9)
8-11: Added Asyncify enum variants.
These new enum entries correctly extend theIntrinsictype to handle asynchronous instantiation and wrapping.
30-30: Indicator for the 'Imports' intrinsic.
Great job introducing a placeholder variant for future expansions of import processing logic.
185-210: Export wrapper ensures re-entrancy checks.
Good job on handling the re-entrance checks byasyncifyAssertNoneState. This helps detect and prevent undesired concurrency issues.
211-231: Import wrapper logic.
Well done weaving in the “stop rewind” and “start unwind” states. The code ensures a correct asynchronous flow.
232-232: Base64 compile fallback.
Providing distinct branches fornodevs browser environments is a thoughtful approach.
404-404: No-op forImportsvariant.
This is consistent with the placeholder. If or when further logic is introduced, add an explanatory comment.
863-866: Proper naming for new asyncify intrinsics.
Mapping all to "asyncifyInstantiate" or "asyncifyWrap*" ensures consistent references in JS.
885-885: Exports the “imports” name.
Exposing the “imports” intrinsic helps maintain clarity around the environment usage.
122-152: IntroduceAsyncifyAsyncInstantiatematch arm.
The logic for capturingasyncifyModules, setting up memory, and returning the instance looks coherent. Verify concurrency if multiple modules are loaded in parallel.✅ Verification successful
Concurrency implementation verified as safe
TheasyncifyModulesarray implementation is designed for sequential module loading and state management. The code uses proper state synchronization throughasyncifyPromiseand performs atomic operations on the array. No parallel module loading patterns were found that could cause race conditions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for concurrency references in intrinsics usage rg -A 10 'intrinsics.*asyncifyModules'Length of output: 40
Script:
#!/bin/bash # Search for asyncifyInstantiate usage patterns rg -A 5 "asyncifyInstantiate\(" # Search for potential parallel loading patterns ast-grep --pattern 'Promise.all($$$)' # Look for any Promise-related code near asyncify rg -A 5 "asyncifyPromise|Promise.*asyncify"Length of output: 8043
crates/js-component-bindgen/src/ts_bindgen.rs (17)
5-5: ImportingAsyncModealigns with PR’s async objectives.
The newAsyncModeusage unifies the approach to asynchronous WebAssembly transpilation.
63-71: Conditional initialization ofasync_importsandasync_exports.
This match expression provides a clear branching forNone,Sync,JavaScriptPromiseIntegration, andAsyncify.
79-80: Assigning asynchronous sets.
Straightforward assignment from the newly parsed data. Looks clean.
410-410: Namespace import approach.
Usinggenerate_interfaceensures each interface is typed. Good approach to keep code consistent.
418-418: Low-camel-case naming.
This is consistent with existing code style for interface import aliases.
437-437: Freestanding function import with default set totrue,declaration=false,is_async=false.
Double-check that you truly intend to skip async for this function, since no async logic is triggered.
451-451: Export interface for WebAssembly instantiation.
Clean separation of the interface creation from the actual export logic.
475-475:export_funcsmethod signature.
Adding adeclaration: boolparameter is a good extension to customize how functions are printed.
481-482: Local assignment forasync_exportsandid_name.
Storing them in local variables is straightforward. Great for clarity.
506-506: Addedis_world_exporttogenerate_interface.
Enables controlling whether to look upasync_exportsorasync_imports. This is a good design extension.
555-560: Identify which functions should be async based on context.
Clear separation: ifis_world_export, pull fromasync_exports; otherwise, fromasync_imports.
577-586: Async detection for interface functions within a world export.
Nicely matches the logic used for top-level exports.
805-806: String interpolation for optional async keyword.
The approach'async 'or''is a neat way to handle optional async.
811-813: Export function name or fallback local name.
Well-structured code to handle disliked identifiers or reserved words. This ensures valid TypeScript definitions.Also applies to: 821-821, 826-826, 828-828, 833-835, 837-839, 848-848, 850-850
887-890:Promise<...>return for async functions.
Properly wrapping the return type ensures accurate TypeScript definitions.
911-914: ClosingPromise<>logic.
Completes the TypeScript return signature for an async function.
390-390: Possible duplication ofgenerate_interfacecalls.
Check if this is invoked in other contexts with the same parameters. If repeated, consider deduplicating logic.crates/js-component-bindgen/src/transpile_bindgen.rs (25)
14-14: Imports for expanded async features look correct.
The addition ofHashSetis consistent with the new async logic.
73-76: Fieldasync_modeis a helpful extension.
Addingasync_mode: Option<AsyncMode>toTranspileOptsbroadens configuration options for asynchronous components.
78-89: EnumAsyncModeappears logically sound.
Defining the explicit modes (Sync, JavaScriptPromiseIntegration, and Asyncify) offers clarity, and using#[default]is a neat way to setSyncmode by default.
156-169: Async matching logic is neatly organized.
The match arms cleanly separate handling forSync,JavaScriptPromiseIntegration, andAsyncify.
177-178: Initialization of new fields is consistent.
Setting upall_core_exported_funcsanduse_asyncifyis correct.
261-261: Implementation block extension is clear.
No issues with opening this newimplblock.
271-272: Preparation for core exported functions is fine.
No functional concerns here.
273-277: Graceful fallback for wrapped exports.
UsingAsyncifyWrapExportwhenuse_asyncifyistrueandWebAssembly.promisingotherwise is a neat approach. Be sure to mention or detect environment support forpromising.
278-289: Conditional wrapping of exported functions is correct.
The logic to determine if a function needs async wrapping or not looks sound.
370-370: Appending core exports to main JS code is valid.
No further suggestions.
439-439: Placeholder usage is safe.
The formatting placeholders seem appropriate and do not introduce security concerns.
470-470: Insertingcore_exported_funcsagain is consistent.
No issues.
518-520: Storing async function info as sets is a good approach.
UsingHashSetforasync_importsandasync_exportshelps with O(1) membership checks.
1096-1101: Asyncify instantiation fallback is well-handled.
Logic ensures we use the correct instantiation intrinsic as needed.
1109-1114: Sync instantiation fallback mirrors async logic well.
No immediate concerns here.
1185-1195:is_asyncdetection for imports is robust.
Double-check that any trailing characters after '@' are handled correctly, but your substring approach looks safe.
1308-1308: Passingis_asyncto bindgen.
Straightforward parameter extension.
1311-1315: Adjusting function close syntax for async vs. non-async.
This conditional approach is correct.
1678-1678: Bindgen now tracks async in signature.
No issues.
1773-1773: Feedingis_asyncintoFunctionBindgen.
Straightforward extension, no immediate concerns.
2054-2068:async_exportsdetection logic mirrorsasync_imports.
Be mindful of partial matches in the presence of '@' marks.
2069-2070: Choosingasyncprefix is concise.
No issues.
2071-2072: Export function references retrieval.
The call toself.core_defis consistent with other usage.
2089-2091: Async static or freestanding function generation.
Allows anasync functiondeclaration ifis_asyncis true.
2097-2097: Prototype assignment for async method looks correct.
No additional suggestions.test/test.js (2)
22-23: Importing new async tests is clear.
No issues with these new import statements.
42-43: Extended test suite for async behavior.
Explicitly callingawait asyncTest()andawait asyncBrowserTest()ensures coverage of new asynchronous scenarios.xtask/src/generate/wasi_types.rs (1)
42-42: Defaultingasync_modetoNone
This preserves existing behavior while allowing future asynchronous expansions.xtask/src/build/jco.rs (1)
88-88: LGTM!The addition of the optional
async_modefield is well-placed and correctly initialized.crates/js-component-bindgen-component/wit/js-component-bindgen.wit (2)
74-77: LGTM!The
async-imports-exportsrecord provides a clear structure for specifying which imports and exports should be asynchronous.
79-85: LGTM!The
async-modevariant is well-documented and provides clear options for sync, JSPI, and Asyncify modes.test/async.js (1)
71-105: LGTM! Comprehensive test coverage for both async modes.The tests thoroughly validate:
- Function type assertions (sync vs async)
- Return value verification
- Proper cleanup
Also applies to: 107-141
src/cmd/opt.js (2)
82-86: LGTM! Good handling of size changes.The code correctly accounts for potential size increases when using asyncify, which is essential for proper optimization.
70-71: Consider validating asyncMode value.While the code handles the 'asyncify' case, it should validate that
asyncModeonly contains expected values.✅ Verification successful
Validation needed but current handling is safe
The codebase consistently uses only two valid values for
asyncMode: 'asyncify' and 'jspi'. While explicit validation would be beneficial, the current implementation safely handles unexpected values by:
- Only adding the
--asyncifyflag when explicitly set to 'asyncify'- Defaulting to 'sync' mode for any unexpected values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any unexpected asyncMode values in the codebase rg -g '*.js' "asyncMode.*['\"](?!asyncify|jspi)[^'\"]+['\"]"Length of output: 329
Script:
#!/bin/bash # Search for asyncMode occurrences echo "=== asyncMode usages ===" rg -g '*.js' -B 2 -A 2 'asyncMode' echo -e "\n=== Type definitions or validation ===" rg -g '*.{js,ts,d.ts}' -B 2 -A 2 '(type.*asyncMode|interface.*asyncMode|enum.*asyncMode)' echo -e "\n=== Documentation mentions ===" rg -g '*.{md,txt}' -B 2 -A 2 'asyncMode'Length of output: 5656
crates/js-component-bindgen-component/src/lib.rs (3)
56-68: Clean implementation of AsyncMode conversion!The From trait implementation correctly maps the async modes while preserving the imports and exports configuration.
93-93: LGTM! Consistent with other option mappings.The async_mode field is correctly added using idiomatic Rust patterns.
180-180: LGTM! Maintains consistency with generate method.The async_mode field is correctly added, maintaining consistency across the codebase.
crates/js-component-bindgen/src/lib.rs (1)
11-11: LGTM! Appropriate public export of AsyncMode.The AsyncMode type is correctly exposed as a public API, maintaining consistency with other exports.
test/async.browser.js (2)
163-172: Well-structured function type verification!The test correctly verifies that sync and async functions have the appropriate types.
197-204: Good configuration for JSPI feature flags!The browser launch configuration correctly enables the experimental JSPI features.
src/jco.js (1)
55-59: Well-structured async options with clear experimental marking!The async-related options are properly marked as EXPERIMENTAL and provide good granular control over async behavior.
test/cli.js (1)
118-154: LGTM! Comprehensive JSPI test coverage.The test thoroughly validates:
- Feature detection for JSPI support
- Correct function type assertions for both sync and async methods
- Proper function behavior for both sync and async calls
src/cmd/transpile.js (3)
17-31: LGTM! Well-structured default async configurations.The default async imports and exports are:
- Comprehensive for WASI operations
- Clearly categorized (IO, streams, etc.)
- Well-documented through descriptive naming
205-210: LGTM! Clear optimization logic for asyncify.The implementation correctly handles optimization requirements for asyncify mode with clear documentation explaining the conditions.
216-224: Address the TODO comment for async path handling.The current implementation has a TODO for async path handling. Consider implementing a more flexible solution:
- Make the async path segment configurable
- Add support for different async modes
- Remove the hardcoded empty string
Would you like me to propose an implementation for the async path handling?
crates/js-component-bindgen/src/function_bindgen.rs (1)
89-89: LGTM! Clean addition of async support.The
is_asyncfield is a clean addition to the FunctionBindgen struct that enables async functionality.
| try { | ||
| await stat(moduleOutputDir); | ||
| } catch (err) { | ||
| if (err && err.code && err.code === "ENOENT") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply optional chaining to check err.code.
The static analysis suggests using optional chaining to simplify the condition.
- if (err && err.code && err.code === "ENOENT") {
+ if (err?.code === "ENOENT") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (err && err.code && err.code === "ENOENT") { | |
| if (err?.code === "ENOENT") { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| /** | ||
| * Set up an async test to be run | ||
| * | ||
| * @param {object} args - Arguments for running the async test | ||
| * @param {function} args.testFn - Arguments for running the async test | ||
| * @param {object} args.jco - JCO-related confguration for running the async test | ||
| * @param {string} [args.jcoBinPath] - path to the jco binary (or a JS script) | ||
| * @param {object} [args.transpile] - configuration related to transpilation | ||
| * @param {string[]} [args.transpile.extraArgs] - arguments to pass along to jco transpilation | ||
| * @param {object} args.component - configuration for an existing component that should be transpiled | ||
| * @param {string} args.component.name - name of the component | ||
| * @param {string} args.component.path - path to the WebAssembly binary for the existing component | ||
| * @param {object[]} args.component.import - imports that should be provided to the module at instantiation time | ||
| * @param {object} args.component.build - configuration for building an ephemeral component to be tested | ||
| * @param {object} args.component.js.source - Javascript source code for a component | ||
| * @param {object} args.component.wit.source - WIT definitions (inlined) for a component | ||
| * @param {object[]} args.component.wit.deps - Dependencies (ex. WASI) that should be included during component build | ||
| */ | ||
| export async function setupAsyncTest(args) { | ||
| const { asyncMode: _asyncMode, testFn, jco, component } = args; | ||
| const asyncMode = _asyncMode || "asyncify"; | ||
| const jcoBinPath = jco?.binPath || jcoPath; | ||
|
|
||
| let componentName = component.name; | ||
| let componentPath = component.path; | ||
| let componentImports = component.imports; | ||
|
|
||
| if (component.path && component.build) { | ||
| throw new Error( | ||
| "Both component.path and component.build should not be specified at the same time", | ||
| ); | ||
| } | ||
|
|
||
| // If this component should be built "just in time" -- i.e. created when this test is run | ||
| let componentBuildCleanup; | ||
| if (component.build) { | ||
| // Optionally use a custom pre-optimized StarlingMonkey engine | ||
| if (env.TEST_CUSTOM_ENGINE_JIT_PATH || env.TEST_CUSTOM_ENGINE_AOT_PATH) { | ||
| log("detected custom engine JIT path"); | ||
| if (component.build.componentizeOpts?.aot) { | ||
| log("detected AOT config"); | ||
| component.build.engine = env.TEST_CUSTOM_ENGINE_AOT_PATH; | ||
| } else { | ||
| log("detected JIT config"); | ||
| component.build.engine = env.TEST_CUSTOM_ENGINE_JIT_PATH; | ||
| } | ||
| } | ||
|
|
||
| // Build the component | ||
| const { name, path, cleanup } = await buildComponent({ | ||
| name: componentName, | ||
| ...component.build, | ||
| }); | ||
|
|
||
| componentBuildCleanup = cleanup; | ||
| componentName = name; | ||
| componentPath = path; | ||
| } | ||
|
|
||
| if (!componentName) { | ||
| throw new Error("invalid/missing component name"); | ||
| } | ||
| if (!componentPath) { | ||
| throw new Error("invalid/missing component path"); | ||
| } | ||
|
|
||
| // Use either a temporary directory or an subfolder in an existing directory, | ||
| // creating it if it doesn't already exist | ||
| const outputDir = component.outputDir | ||
| ? component.outputDir | ||
| : await getTmpDir(); | ||
|
|
||
| // Build out the whole-test cleanup function | ||
| let cleanup = async () => { | ||
| log("[cleanup] cleaning up component..."); | ||
| if (componentBuildCleanup) { | ||
| try { | ||
| await componentBuildCleanup(); | ||
| } catch {} | ||
| } | ||
| try { | ||
| await rm(outputDir, { recursive: true }); | ||
| } catch {} | ||
| }; | ||
|
|
||
| // Return early if the test was intended to run on JSPI but JSPI is not enabled | ||
| if (asyncMode == "jspi" && typeof WebAssembly?.Suspending !== "function") { | ||
| let nodeMajorVersion = parseInt(version.replace("v","").split(".")[0]); | ||
| if (nodeMajorVersion < 23) { | ||
| throw new Error("NodeJS versions <23 does not support JSPI integration, please use a NodeJS version >=23"); | ||
| } | ||
| await cleanup(); | ||
| throw new Error( | ||
| "JSPI async type skipped, but JSPI was not enabled -- please ensure test is run from an environment with JSPI integration (ex. node with the --experimental-wasm-jspi flag)", | ||
| ); | ||
| } | ||
|
|
||
| // Build a directory for the transpiled component output to be put in | ||
| // (possibly inside the passed in outputDir) | ||
| const moduleOutputDir = join(outputDir, component.name); | ||
| try { | ||
| await stat(moduleOutputDir); | ||
| } catch (err) { | ||
| if (err && err.code && err.code === "ENOENT") { | ||
| await mkdir(moduleOutputDir); | ||
| } | ||
| } | ||
|
|
||
| const transpileOpts = { | ||
| name: componentName, | ||
| minify: true, | ||
| validLiftingOptimization: true, | ||
| tlaCompat: true, | ||
| optimize: false, | ||
| base64Cutoff: 0, | ||
| instantiation: "async", | ||
| asyncMode, | ||
| wasiShim: true, | ||
| outDir: moduleOutputDir, | ||
| ...(jco?.transpile?.extraArgs || {}), | ||
| }; | ||
|
|
||
| // If we used a pre-optimized build, then we can set that before transpiling | ||
| if (["yes", "true"].includes(env.TEST_CUSTOM_ENGINE_PREOPTIMIZED)) { | ||
| log("using preoptimized engine build!"); | ||
| transpileOpts.preoptimized = true; | ||
| } | ||
|
|
||
| const componentBytes = await readFile(componentPath); | ||
|
|
||
| // Perform transpilation, write out files | ||
| const { files } = await transpile(componentBytes, transpileOpts); | ||
| await Promise.all( | ||
| Object.entries(files).map(async ([name, file]) => { | ||
| await mkdir(dirname(name), { recursive: true }); | ||
| await writeFile(name, file); | ||
| }), | ||
| ); | ||
|
|
||
| // Write a minimal package.json | ||
| await writeFile( | ||
| `${moduleOutputDir}/package.json`, | ||
| JSON.stringify({ type: "module" }), | ||
| ); | ||
|
|
||
| // TODO: DEBUG module import not working, file is missing! | ||
| // log("WROTE EVERYTHING:", moduleOutputDir); | ||
| // await new Promise(resolve => setTimeout(resolve, 60_000)); | ||
|
|
||
| // Import the transpiled JS | ||
| const esModuleOutputPath = join(moduleOutputDir, `${componentName}.js`); | ||
| const esModuleSourcePathURL = pathToFileURL(esModuleOutputPath); | ||
| const module = await import(esModuleSourcePathURL); | ||
|
|
||
| // TODO: DEBUG module import not working, file is missing! | ||
| // log("PRE INSTANTIATION", { moduleOutputDir }); | ||
| // await new Promise(resolve => setTimeout(resolve, 60_000_000)); | ||
|
|
||
| // Optionally instantiate the module | ||
| // | ||
| // It's useful to be able to skip instantiation of the instantiation should happen | ||
| // elsewhere (ex. in a browser window) | ||
| let instance = null; | ||
| if (!component.skipInstantiation) { | ||
| instance = await module.instantiate(undefined, componentImports || {}); | ||
| } | ||
|
|
||
| return { | ||
| module, | ||
| esModuleOutputPath, | ||
| esModuleSourcePathURL, | ||
| esModuleRelativeSourcePath: relative(outputDir, esModuleOutputPath), | ||
| instance, | ||
| cleanup, | ||
| outputDir, | ||
| component: { | ||
| name: componentName, | ||
| path: componentPath, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused testFn parameter
The testFn parameter is documented but never used in the implementation, and no callers provide this parameter. Consider removing it from both the function signature and JSDoc.
🔗 Analysis chain
Validate presence of testFn within setupAsyncTest.
This function expects args.testFn but never uses it or verifies its type. Consider ensuring this property is set or used if relevant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Script to check for direct references to `testFn` usage in the codebase
rg -A 3 'setupAsyncTest.*testFn'
Length of output: 34
Script:
#!/bin/bash
# Find all imports and usages of setupAsyncTest
rg -A 5 "setupAsyncTest" --type js
# Find test files that might contain example usage
fd -e test.js -e spec.js -x cat {}
Length of output: 2600
🧰 Tools
🪛 Biome (1.9.4)
[error] 193-193: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| strictEqual(stderr, ""); | ||
| await writeFile( | ||
| `${outDir}/package.json`, | ||
| JSON.stringify({ type: "module" }) | ||
| JSON.stringify({ type: "module" }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent path handling methods.
Replace string concatenation with path.join() or path.resolve() for better cross-platform compatibility. For example:
-await writeFile(
- `${outDir}/package.json`,
- JSON.stringify({ type: "module" }),
-);
+await writeFile(
+ path.join(outDir, "package.json"),
+ JSON.stringify({ type: "module" }),
+);Also applies to: 137-140, 174-177, 293-297, 318-321
package.json
Outdated
| "lint": "eslint -c eslintrc.cjs src/**/*.js packages/*/lib/**/*.js", | ||
| "test:lts": "mocha -u tdd test/test.js --timeout 120000", | ||
| "test": "node --stack-trace-limit=100 node_modules/mocha/bin/mocha.js -u tdd test/test.js --timeout 120000", | ||
| "test": "node --experimental-wasm-jspi --stack-trace-limit=100 node_modules/mocha/bin/mocha.js -u tdd test/test.js --timeout 120000", |
There was a problem hiding this comment.
💡 Codebase verification
Document JSPI requirement in README.md
The --experimental-wasm-jspi flag is supported in the CI environment (Node.js v22.9.0), but this requirement should be documented in README.md to inform developers about:
- Node.js version requirements for running tests
- The experimental JSPI flag needed for test execution
🔗 Analysis chain
Document JSPI requirement and verify CI compatibility.
The addition of --experimental-wasm-jspi flag enables JSPI testing but:
- Should be documented in README.md
- May affect CI/CD pipelines
- Should have a fallback for environments without JSPI support
Let's check if the flag is supported in the CI environment:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version and JSPI support in CI
node --version
node --experimental-wasm-jspi --eval "console.log('JSPI supported')" || echo "JSPI not supported"
Length of output: 116
Script:
#!/bin/bash
# Check README.md for JSPI documentation
rg -i "jspi|webassembly|wasm" README.md
Length of output: 2922
| <script type="module"> | ||
| // OK/Error states for the test function | ||
| // (the test harness should check for body contents) | ||
| function ok(msg) { document.body.innerHTML = typeof msg === "object" ? JSON.stringify(msg) : `${err}`; } |
There was a problem hiding this comment.
Fix incorrect variable reference in ok function.
There's a bug where the function uses err instead of msg in the template literal.
-function ok(msg) { document.body.innerHTML = typeof msg === "object" ? JSON.stringify(msg) : `${err}`; }
+function ok(msg) { document.body.innerHTML = typeof msg === "object" ? JSON.stringify(msg) : `${msg}`; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function ok(msg) { document.body.innerHTML = typeof msg === "object" ? JSON.stringify(msg) : `${err}`; } | |
| function ok(msg) { document.body.innerHTML = typeof msg === "object" ? JSON.stringify(msg) : `${msg}`; } |
| let count = 0; | ||
| const wake = setInterval(() => { | ||
| count += 1; | ||
| }, 50); | ||
|
|
||
| // Run the exported function (which will call the async host import above), | ||
| // get response | ||
| const responseText = await fn(); | ||
|
|
||
| if (count <= 20) { | ||
| err(`ERROR setTimeout did not run unimpeded, only completed [${count}] 50ms increments (expected ~20)`); | ||
| } | ||
|
|
||
| ok({ responseText }); |
There was a problem hiding this comment.
Add cleanup for setInterval.
The setInterval timer should be cleared after the test completes or fails.
let count = 0;
const wake = setInterval(() => {
count += 1;
}, 50);
+try {
// Run the exported function (which will call the async host import above),
// get response
const responseText = await fn();
if (count <= 20) {
err(`ERROR setTimeout did not run unimpeded, only completed [${count}] 50ms increments (expected ~20)`);
}
ok({ responseText });
+} finally {
+ clearInterval(wake);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let count = 0; | |
| const wake = setInterval(() => { | |
| count += 1; | |
| }, 50); | |
| // Run the exported function (which will call the async host import above), | |
| // get response | |
| const responseText = await fn(); | |
| if (count <= 20) { | |
| err(`ERROR setTimeout did not run unimpeded, only completed [${count}] 50ms increments (expected ~20)`); | |
| } | |
| ok({ responseText }); | |
| let count = 0; | |
| const wake = setInterval(() => { | |
| count += 1; | |
| }, 50); | |
| try { | |
| // Run the exported function (which will call the async host import above), | |
| // get response | |
| const responseText = await fn(); | |
| if (count <= 20) { | |
| err(`ERROR setTimeout did not run unimpeded, only completed [${count}] 50ms increments (expected ~20)`); | |
| } | |
| ok({ responseText }); | |
| } finally { | |
| clearInterval(wake); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
test/async.js (4)
21-53: Well-implemented test suite setup and teardown with proper resource management!Consider logging cleanup errors for debugging purposes, even though they're safely caught.
suiteTeardown(async function () { try { await rm(tmpDir, { recursive: true }); - } catch {} + } catch (error) { + console.warn('Cleanup warning:', error.message); + } });
54-69: Enhance test coverage with additional assertions.While the basic transpilation test works, consider adding more comprehensive checks:
- Validate the structure of generated exports
- Check for presence of async/await syntax in output
- Verify error cases
test("Transpile async", async () => { const name = "flavorful"; const { stderr } = await exec( jcoPath, "transpile", `test/fixtures/components/${name}.component.wasm`, "--no-wasi-shim", "--name", name, "-o", outDir ); strictEqual(stderr, ""); const source = await readFile(`${outDir}/${name}.js`); + const sourceText = source.toString(); ok(source.toString().includes("export { test")); + ok(sourceText.includes('async'), 'Should contain async keyword'); + ok(sourceText.includes('await'), 'Should contain await keyword'); + ok(!sourceText.includes('undefined'), 'Should not contain undefined exports'); });
71-107: Add error case testing for JSPI implementation.The happy path is well tested, but consider adding error cases:
if (typeof WebAssembly?.Suspending === "function") { test("Transpile async (NodeJS, JSPI)", async () => { const { instance, cleanup, component } = await setupAsyncTest({ asyncMode: "jspi", component: { name: "async_call", path: resolve("test/fixtures/components/async_call.component.wasm"), imports: { 'something:test/test-interface': { callAsync: async () => "called async", callSync: () => "called sync", }, }, }, // ... }); + // Test error handling + await assert.rejects( + async () => instance.runAsync(null), + /Invalid argument/, + 'Should handle invalid arguments' + ); + + try { + await instance.runAsync(); + } catch (error) { + fail('Should not throw on valid call'); + } await cleanup(); }); }
109-143: Add edge cases to Asyncify test implementation.Consider testing these additional scenarios:
test("Transpile async (NodeJS, Asyncify)", async () => { const { instance, cleanup } = await setupAsyncTest({ // ... existing setup ... }); strictEqual(instance.runSync instanceof AsyncFunction, false); strictEqual(instance.runAsync instanceof AsyncFunction, true); strictEqual(instance.runSync(), "called sync"); strictEqual(await instance.runAsync(), "called async"); + // Test concurrent calls + const results = await Promise.all([ + instance.runAsync(), + instance.runAsync(), + instance.runAsync() + ]); + deepStrictEqual(results, [ + "called async", + "called async", + "called async" + ], "Should handle concurrent async calls"); + + // Test mixing sync and async calls + const mixedResults = await Promise.all([ + Promise.resolve(instance.runSync()), + instance.runAsync() + ]); + deepStrictEqual(mixedResults, [ + "called sync", + "called async" + ], "Should handle mixed sync/async calls"); await cleanup(); });test/async.browser.js (2)
44-55: Enhance cleanup error handling for browser tests.Consider adding more robust cleanup with logging and specific error handling:
suiteTeardown(async function () { try { await rm(tmpDir, { recursive: true }); - } catch {} + } catch (error) { + console.warn('Suite teardown warning:', error.message); + } }); teardown(async function () { try { await rm(outDir, { recursive: true }); await rm(outFile); - } catch {} + } catch (error) { + console.warn('Test teardown warning:', error.message); + } });
92-107: Reduce code duplication in server and browser setup.Extract common setup code into utility functions:
+ async function setupTestServer(outputDir) { + return await startTestWebServer({ + routes: [ + { + urlPrefix: "/transpiled/", + basePathURL: pathToFileURL(`${outputDir}/`), + }, + { basePathURL: import.meta.url }, + ], + }); + } + + async function setupTestBrowser(jspi = false) { + const options = jspi ? { + args: [ + "--enable-experimental-webassembly-jspi", + "--flag-switches-begin", + "--enable-features=WebAssemblyExperimentalJSPI", + "--flag-switches-end", + ], + } : {}; + return await puppeteer.launch(options); + }Then use these utility functions in both tests to reduce duplication and improve maintainability.
Also applies to: 181-195
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/async.browser.js(1 hunks)test/async.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Jco Test Suite (windows-latest, 20.x)
- GitHub Check: Jco Node.js WASI Conformance Tests (windows-latest, latest)
- GitHub Check: Jco Test Suite (ubuntu-22.04, 18.x)
- GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, 18.x)
🔇 Additional comments (1)
test/async.js (1)
1-20: Well-structured test setup with proper imports and configurations!The setup includes appropriate Node.js modules, helper functions, and handles experimental features correctly.
| test("Transpile async (browser, asyncify)", async () => { | ||
| const componentName = "async-call"; | ||
| const { | ||
| instance, | ||
| cleanup: componentCleanup, | ||
| outputDir, | ||
| } = await setupAsyncTest({ | ||
| asyncMode: "asyncify", | ||
| component: { | ||
| name: "async_call", | ||
| path: resolve("test/fixtures/components/async_call.component.wasm"), | ||
| imports: { | ||
| "something:test/test-interface": { | ||
| callAsync: async () => "called async", | ||
| callSync: () => "called sync", | ||
| }, | ||
| }, | ||
| }, | ||
| jco: { | ||
| transpile: { | ||
| extraArgs: { | ||
| asyncImports: ["something:test/test-interface#call-async"], | ||
| asyncExports: ["run-async"], | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| const moduleName = componentName.toLowerCase().replaceAll("-", "_"); | ||
| const moduleRelPath = `${moduleName}/${moduleName}.js`; | ||
|
|
||
| // Start a test web server | ||
| const { | ||
| server, | ||
| serverPort, | ||
| cleanup: webServerCleanup, | ||
| } = await startTestWebServer({ | ||
| routes: [ | ||
| // NOTE: the goal here is to serve relative paths via the browser hash | ||
| // | ||
| // (1) browser visits test page (served by test web server) | ||
| // (2) browser requests component itself by looking at URL hash fragment | ||
| // (i.e. "#transpiled:async_call/async_call.js" -> , "/transpiled/async_call/async_call.js") | ||
| // (i.e. "/transpiled/async_call/async_call.js" -> file read of /tmp/xxxxxx/async_call/async_call.js) | ||
| { | ||
| urlPrefix: "/transpiled/", | ||
| basePathURL: pathToFileURL(`${outputDir}/`), | ||
| }, | ||
| // Serve all other files (ex. the initial HTML for the page) | ||
| { basePathURL: import.meta.url }, | ||
| ], | ||
| }); | ||
|
|
||
| // Start a browser to visit the test server | ||
| const browser = await puppeteer.launch(); | ||
|
|
||
| // Load the test page in the browser, which will trigger tests against | ||
| // the component and/or related browser polyfills | ||
| const { | ||
| page, | ||
| output: { json }, | ||
| } = await loadTestPage({ | ||
| browser, | ||
| serverPort, | ||
| path: "fixtures/browser/test-pages/something__test.async.html", | ||
| hash: `transpiled:${moduleRelPath}`, | ||
| }); | ||
|
|
||
| // Check the output expected to be returned from handle of the | ||
| // guest export (this depends on the component) | ||
| deepStrictEqual(json, { responseText: "callAsync" }); | ||
|
|
||
| await browser.close(); | ||
| await webServerCleanup(); | ||
| await componentCleanup(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling for browser and server operations.
The test setup is good, but browser and server operations need better error handling:
test("Transpile async (browser, asyncify)", async () => {
+ let browser;
+ let cleanup = async () => {};
+
try {
const componentName = "async-call";
const {
instance,
cleanup: componentCleanup,
outputDir,
} = await setupAsyncTest({
// ... existing setup ...
});
+ cleanup = componentCleanup;
// ... server setup ...
// Start a browser to visit the test server
- const browser = await puppeteer.launch();
+ browser = await puppeteer.launch();
const {
page,
output: { json },
} = await loadTestPage({
// ... existing setup ...
});
deepStrictEqual(json, { responseText: "callAsync" });
- await browser.close();
- await webServerCleanup();
- await componentCleanup();
+ } catch (error) {
+ console.error('Test failed:', error);
+ throw error;
+ } finally {
+ if (browser) {
+ await browser.close().catch(console.warn);
+ }
+ await webServerCleanup().catch(console.warn);
+ await cleanup().catch(console.warn);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("Transpile async (browser, asyncify)", async () => { | |
| const componentName = "async-call"; | |
| const { | |
| instance, | |
| cleanup: componentCleanup, | |
| outputDir, | |
| } = await setupAsyncTest({ | |
| asyncMode: "asyncify", | |
| component: { | |
| name: "async_call", | |
| path: resolve("test/fixtures/components/async_call.component.wasm"), | |
| imports: { | |
| "something:test/test-interface": { | |
| callAsync: async () => "called async", | |
| callSync: () => "called sync", | |
| }, | |
| }, | |
| }, | |
| jco: { | |
| transpile: { | |
| extraArgs: { | |
| asyncImports: ["something:test/test-interface#call-async"], | |
| asyncExports: ["run-async"], | |
| }, | |
| }, | |
| }, | |
| }); | |
| const moduleName = componentName.toLowerCase().replaceAll("-", "_"); | |
| const moduleRelPath = `${moduleName}/${moduleName}.js`; | |
| // Start a test web server | |
| const { | |
| server, | |
| serverPort, | |
| cleanup: webServerCleanup, | |
| } = await startTestWebServer({ | |
| routes: [ | |
| // NOTE: the goal here is to serve relative paths via the browser hash | |
| // | |
| // (1) browser visits test page (served by test web server) | |
| // (2) browser requests component itself by looking at URL hash fragment | |
| // (i.e. "#transpiled:async_call/async_call.js" -> , "/transpiled/async_call/async_call.js") | |
| // (i.e. "/transpiled/async_call/async_call.js" -> file read of /tmp/xxxxxx/async_call/async_call.js) | |
| { | |
| urlPrefix: "/transpiled/", | |
| basePathURL: pathToFileURL(`${outputDir}/`), | |
| }, | |
| // Serve all other files (ex. the initial HTML for the page) | |
| { basePathURL: import.meta.url }, | |
| ], | |
| }); | |
| // Start a browser to visit the test server | |
| const browser = await puppeteer.launch(); | |
| // Load the test page in the browser, which will trigger tests against | |
| // the component and/or related browser polyfills | |
| const { | |
| page, | |
| output: { json }, | |
| } = await loadTestPage({ | |
| browser, | |
| serverPort, | |
| path: "fixtures/browser/test-pages/something__test.async.html", | |
| hash: `transpiled:${moduleRelPath}`, | |
| }); | |
| // Check the output expected to be returned from handle of the | |
| // guest export (this depends on the component) | |
| deepStrictEqual(json, { responseText: "callAsync" }); | |
| await browser.close(); | |
| await webServerCleanup(); | |
| await componentCleanup(); | |
| }); | |
| test("Transpile async (browser, asyncify)", async () => { | |
| let browser; | |
| let cleanup = async () => {}; | |
| try { | |
| const componentName = "async-call"; | |
| const { | |
| instance, | |
| cleanup: componentCleanup, | |
| outputDir, | |
| } = await setupAsyncTest({ | |
| asyncMode: "asyncify", | |
| component: { | |
| name: "async_call", | |
| path: resolve("test/fixtures/components/async_call.component.wasm"), | |
| imports: { | |
| "something:test/test-interface": { | |
| callAsync: async () => "called async", | |
| callSync: () => "called sync", | |
| }, | |
| }, | |
| }, | |
| jco: { | |
| transpile: { | |
| extraArgs: { | |
| asyncImports: ["something:test/test-interface#call-async"], | |
| asyncExports: ["run-async"], | |
| }, | |
| }, | |
| }, | |
| }); | |
| cleanup = componentCleanup; | |
| const moduleName = componentName.toLowerCase().replaceAll("-", "_"); | |
| const moduleRelPath = `${moduleName}/${moduleName}.js`; | |
| // Start a test web server | |
| const { | |
| server, | |
| serverPort, | |
| cleanup: webServerCleanup, | |
| } = await startTestWebServer({ | |
| routes: [ | |
| // NOTE: the goal here is to serve relative paths via the browser hash | |
| // | |
| // (1) browser visits test page (served by test web server) | |
| // (2) browser requests component itself by looking at URL hash fragment | |
| // (i.e. "#transpiled:async_call/async_call.js" -> , "/transpiled/async_call/async_call.js") | |
| // (i.e. "/transpiled/async_call/async_call.js" -> file read of /tmp/xxxxxx/async_call/async_call.js) | |
| { | |
| urlPrefix: "/transpiled/", | |
| basePathURL: pathToFileURL(`${outputDir}/`), | |
| }, | |
| // Serve all other files (ex. the initial HTML for the page) | |
| { basePathURL: import.meta.url }, | |
| ], | |
| }); | |
| // Start a browser to visit the test server | |
| browser = await puppeteer.launch(); | |
| // Load the test page in the browser, which will trigger tests against | |
| // the component and/or related browser polyfills | |
| const { | |
| page, | |
| output: { json }, | |
| } = await loadTestPage({ | |
| browser, | |
| serverPort, | |
| path: "fixtures/browser/test-pages/something__test.async.html", | |
| hash: `transpiled:${moduleRelPath}`, | |
| }); | |
| // Check the output expected to be returned from handle of the | |
| // guest export (this depends on the component) | |
| deepStrictEqual(json, { responseText: "callAsync" }); | |
| } catch (error) { | |
| console.error('Test failed:', error); | |
| throw error; | |
| } finally { | |
| if (browser) { | |
| await browser.close().catch(console.warn); | |
| } | |
| await webServerCleanup().catch(console.warn); | |
| await cleanup().catch(console.warn); | |
| } | |
| }); |
| if (typeof WebAssembly?.Suspending === "function") { | ||
| test("Transpile async (browser, JSPI)", async () => { | ||
| const componentName = "async-call"; | ||
| const { | ||
| instance, | ||
| cleanup: componentCleanup, | ||
| outputDir, | ||
| } = await setupAsyncTest({ | ||
| asyncMode: "jspi", | ||
| component: { | ||
| name: "async_call", | ||
| path: resolve("test/fixtures/components/async_call.component.wasm"), | ||
| imports: { | ||
| "something:test/test-interface": { | ||
| callAsync: async () => "called async", | ||
| callSync: () => "called sync", | ||
| }, | ||
| }, | ||
| }, | ||
| jco: { | ||
| transpile: { | ||
| extraArgs: { | ||
| asyncImports: ["something:test/test-interface#call-async"], | ||
| asyncExports: ["run-async"], | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
| const moduleName = componentName.toLowerCase().replaceAll("-", "_"); | ||
| const moduleRelPath = `${moduleName}/${moduleName}.js`; | ||
|
|
||
| strictEqual( | ||
| instance.runSync instanceof AsyncFunction, | ||
| false, | ||
| "runSync() should be a sync function", | ||
| ); | ||
| strictEqual( | ||
| instance.runAsync instanceof AsyncFunction, | ||
| true, | ||
| "runAsync() should be an async function", | ||
| ); | ||
|
|
||
| // Start a test web server | ||
| const { | ||
| server, | ||
| serverPort, | ||
| cleanup: webServerCleanup, | ||
| } = await startTestWebServer({ | ||
| routes: [ | ||
| // NOTE: the goal here is to serve relative paths via the browser hash | ||
| // | ||
| // (1) browser visits test page (served by test web server) | ||
| // (2) browser requests component itself by looking at URL hash fragment | ||
| // (i.e. "#transpiled:async_call/async_call.js" -> , "/transpiled/async_call/async_call.js") | ||
| // (i.e. "/transpiled/async_call/async_call.js" -> file read of /tmp/xxxxxx/async_call/async_call.js) | ||
| { | ||
| urlPrefix: "/transpiled/", | ||
| basePathURL: pathToFileURL(`${outputDir}/`), | ||
| }, | ||
| // Serve all other files (ex. the initial HTML for the page) | ||
| { basePathURL: import.meta.url }, | ||
| ], | ||
| }); | ||
|
|
||
| // Start a browser to visit the test server | ||
| const browser = await puppeteer.launch({ | ||
| args: [ | ||
| "--enable-experimental-webassembly-jspi", | ||
| "--flag-switches-begin", | ||
| "--enable-features=WebAssemblyExperimentalJSPI", | ||
| "--flag-switches-end", | ||
| ], | ||
| }); | ||
|
|
||
| // Load the test page in the browser, which will trigger tests against | ||
| // the component and/or related browser polyfills | ||
| const { | ||
| page, | ||
| output: { json }, | ||
| } = await loadTestPage({ | ||
| browser, | ||
| serverPort, | ||
| path: "fixtures/browser/test-pages/something__test.async.html", | ||
| hash: `transpiled:${moduleRelPath}`, | ||
| }); | ||
|
|
||
| // Check the output expected to be returned from handle of the | ||
| // guest export (this depends on the component) | ||
| deepStrictEqual(json, { responseText: "callAsync" }); | ||
|
|
||
| await browser.close(); | ||
| await webServerCleanup(); | ||
| await componentCleanup(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and add validation for JSPI browser test.
The test needs similar error handling improvements as the Asyncify test, plus additional JSPI-specific validations:
if (typeof WebAssembly?.Suspending === "function") {
test("Transpile async (browser, JSPI)", async () => {
+ let browser;
+ let cleanup = async () => {};
+
try {
const componentName = "async-call";
const {
instance,
cleanup: componentCleanup,
outputDir,
} = await setupAsyncTest({
// ... existing setup ...
});
+ cleanup = componentCleanup;
// ... existing assertions ...
// Start a browser to visit the test server
- const browser = await puppeteer.launch({
+ browser = await puppeteer.launch({
args: [
"--enable-experimental-webassembly-jspi",
"--flag-switches-begin",
"--enable-features=WebAssemblyExperimentalJSPI",
"--flag-switches-end",
],
});
+ // Verify JSPI is enabled in the browser
+ const page = await browser.newPage();
+ const jspiEnabled = await page.evaluate(() => {
+ return typeof WebAssembly?.Suspending === 'function';
+ });
+ ok(jspiEnabled, 'JSPI should be enabled in the browser');
// ... existing test code ...
- await browser.close();
- await webServerCleanup();
- await componentCleanup();
+ } catch (error) {
+ console.error('Test failed:', error);
+ throw error;
+ } finally {
+ if (browser) {
+ await browser.close().catch(console.warn);
+ }
+ await webServerCleanup().catch(console.warn);
+ await cleanup().catch(console.warn);
+ }
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof WebAssembly?.Suspending === "function") { | |
| test("Transpile async (browser, JSPI)", async () => { | |
| const componentName = "async-call"; | |
| const { | |
| instance, | |
| cleanup: componentCleanup, | |
| outputDir, | |
| } = await setupAsyncTest({ | |
| asyncMode: "jspi", | |
| component: { | |
| name: "async_call", | |
| path: resolve("test/fixtures/components/async_call.component.wasm"), | |
| imports: { | |
| "something:test/test-interface": { | |
| callAsync: async () => "called async", | |
| callSync: () => "called sync", | |
| }, | |
| }, | |
| }, | |
| jco: { | |
| transpile: { | |
| extraArgs: { | |
| asyncImports: ["something:test/test-interface#call-async"], | |
| asyncExports: ["run-async"], | |
| }, | |
| }, | |
| }, | |
| }); | |
| const moduleName = componentName.toLowerCase().replaceAll("-", "_"); | |
| const moduleRelPath = `${moduleName}/${moduleName}.js`; | |
| strictEqual( | |
| instance.runSync instanceof AsyncFunction, | |
| false, | |
| "runSync() should be a sync function", | |
| ); | |
| strictEqual( | |
| instance.runAsync instanceof AsyncFunction, | |
| true, | |
| "runAsync() should be an async function", | |
| ); | |
| // Start a test web server | |
| const { | |
| server, | |
| serverPort, | |
| cleanup: webServerCleanup, | |
| } = await startTestWebServer({ | |
| routes: [ | |
| // NOTE: the goal here is to serve relative paths via the browser hash | |
| // | |
| // (1) browser visits test page (served by test web server) | |
| // (2) browser requests component itself by looking at URL hash fragment | |
| // (i.e. "#transpiled:async_call/async_call.js" -> , "/transpiled/async_call/async_call.js") | |
| // (i.e. "/transpiled/async_call/async_call.js" -> file read of /tmp/xxxxxx/async_call/async_call.js) | |
| { | |
| urlPrefix: "/transpiled/", | |
| basePathURL: pathToFileURL(`${outputDir}/`), | |
| }, | |
| // Serve all other files (ex. the initial HTML for the page) | |
| { basePathURL: import.meta.url }, | |
| ], | |
| }); | |
| // Start a browser to visit the test server | |
| const browser = await puppeteer.launch({ | |
| args: [ | |
| "--enable-experimental-webassembly-jspi", | |
| "--flag-switches-begin", | |
| "--enable-features=WebAssemblyExperimentalJSPI", | |
| "--flag-switches-end", | |
| ], | |
| }); | |
| // Load the test page in the browser, which will trigger tests against | |
| // the component and/or related browser polyfills | |
| const { | |
| page, | |
| output: { json }, | |
| } = await loadTestPage({ | |
| browser, | |
| serverPort, | |
| path: "fixtures/browser/test-pages/something__test.async.html", | |
| hash: `transpiled:${moduleRelPath}`, | |
| }); | |
| // Check the output expected to be returned from handle of the | |
| // guest export (this depends on the component) | |
| deepStrictEqual(json, { responseText: "callAsync" }); | |
| await browser.close(); | |
| await webServerCleanup(); | |
| await componentCleanup(); | |
| }); | |
| } | |
| if (typeof WebAssembly?.Suspending === "function") { | |
| test("Transpile async (browser, JSPI)", async () => { | |
| let browser; | |
| let cleanup = async () => {}; | |
| try { | |
| const componentName = "async-call"; | |
| const { | |
| instance, | |
| cleanup: componentCleanup, | |
| outputDir, | |
| } = await setupAsyncTest({ | |
| asyncMode: "jspi", | |
| component: { | |
| name: "async_call", | |
| path: resolve("test/fixtures/components/async_call.component.wasm"), | |
| imports: { | |
| "something:test/test-interface": { | |
| callAsync: async () => "called async", | |
| callSync: () => "called sync", | |
| }, | |
| }, | |
| }, | |
| jco: { | |
| transpile: { | |
| extraArgs: { | |
| asyncImports: ["something:test/test-interface#call-async"], | |
| asyncExports: ["run-async"], | |
| }, | |
| }, | |
| }, | |
| }); | |
| cleanup = componentCleanup; | |
| const moduleName = componentName.toLowerCase().replaceAll("-", "_"); | |
| const moduleRelPath = `${moduleName}/${moduleName}.js`; | |
| strictEqual( | |
| instance.runSync instanceof AsyncFunction, | |
| false, | |
| "runSync() should be a sync function", | |
| ); | |
| strictEqual( | |
| instance.runAsync instanceof AsyncFunction, | |
| true, | |
| "runAsync() should be an async function", | |
| ); | |
| // Start a test web server | |
| const { | |
| server, | |
| serverPort, | |
| cleanup: webServerCleanup, | |
| } = await startTestWebServer({ | |
| routes: [ | |
| // NOTE: the goal here is to serve relative paths via the browser hash | |
| // | |
| // (1) browser visits test page (served by test web server) | |
| // (2) browser requests component itself by looking at URL hash fragment | |
| // (i.e. "#transpiled:async_call/async_call.js" -> , "/transpiled/async_call/async_call.js") | |
| // (i.e. "/transpiled/async_call/async_call.js" -> file read of /tmp/xxxxxx/async_call/async_call.js) | |
| { | |
| urlPrefix: "/transpiled/", | |
| basePathURL: pathToFileURL(`${outputDir}/`), | |
| }, | |
| // Serve all other files (ex. the initial HTML for the page) | |
| { basePathURL: import.meta.url }, | |
| ], | |
| }); | |
| // Start a browser to visit the test server | |
| browser = await puppeteer.launch({ | |
| args: [ | |
| "--enable-experimental-webassembly-jspi", | |
| "--flag-switches-begin", | |
| "--enable-features=WebAssemblyExperimentalJSPI", | |
| "--flag-switches-end", | |
| ], | |
| }); | |
| // Verify JSPI is enabled in the browser | |
| const page = await browser.newPage(); | |
| const jspiEnabled = await page.evaluate(() => { | |
| return typeof WebAssembly?.Suspending === 'function'; | |
| }); | |
| ok(jspiEnabled, 'JSPI should be enabled in the browser'); | |
| // Load the test page in the browser, which will trigger tests against | |
| // the component and/or related browser polyfills | |
| const { | |
| page, | |
| output: { json }, | |
| } = await loadTestPage({ | |
| browser, | |
| serverPort, | |
| path: "fixtures/browser/test-pages/something__test.async.html", | |
| hash: `transpiled:${moduleRelPath}`, | |
| }); | |
| // Check the output expected to be returned from handle of the | |
| // guest export (this depends on the component) | |
| deepStrictEqual(json, { responseText: "callAsync" }); | |
| } catch (error) { | |
| console.error('Test failed:', error); | |
| throw error; | |
| } finally { | |
| if (browser) { | |
| await browser.close().catch(console.warn); | |
| } | |
| await webServerCleanup().catch(console.warn); | |
| await cleanup().catch(console.warn); | |
| } | |
| }); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cli.js (2)
156-190: Consider reducing duplication between JSPI and Asyncify tests.While the implementation is correct, there's significant code duplication between the JSPI and Asyncify tests. Consider extracting common test logic into a helper function.
+async function testAsyncMode(mode, name = "async_call") { + const { stderr } = await exec( + jcoPath, + "transpile", + `test/fixtures/components/${name}.component.wasm`, + `--name=${name}`, + "--valid-lifting-optimization", + "--tla-compat", + "--instantiation=async", + "--base64-cutoff=0", + `--async-mode=${mode}`, + "--async-imports=something:test/test-interface#call-async", + "--async-exports=run-async", + "-o", + outDir, + ); + strictEqual(stderr, ""); + await writeFile( + `${outDir}/package.json`, + JSON.stringify({ type: "module" }), + ); + const m = await import(`${pathToFileURL(outDir)}/${name}.js`); + const inst = await m.instantiate(undefined, { + "something:test/test-interface": { + callAsync: async () => "called async", + callSync: () => "called sync", + }, + }); + strictEqual(inst.runSync instanceof AsyncFunction, false); + strictEqual(inst.runAsync instanceof AsyncFunction, true); + + strictEqual(inst.runSync(), "called sync"); + strictEqual(await inst.runAsync(), "called async"); +} + +if (typeof WebAssembly.Suspending === "function") { + test("Transpile with Async Mode for JSPI", () => testAsyncMode("jspi")); +} + +test("Transpile with Async Mode for Asyncify", () => testAsyncMode("asyncify"));
654-681: Consider adding cache limits and invalidation.The caching implementation is clean and well-documented. However, consider:
- Adding a maximum cache size to prevent memory issues
- Implementing cache invalidation for old entries
- Adding error handling for cache write failures
// Cache of componentize byte outputs -const CACHE_COMPONENTIZE_OUTPUT = {}; +const CACHE_COMPONENTIZE_OUTPUT = new Map(); +const MAX_CACHE_SIZE = 100; // Maximum number of entries + +function clearOldestCacheEntry() { + const firstKey = CACHE_COMPONENTIZE_OUTPUT.keys().next().value; + CACHE_COMPONENTIZE_OUTPUT.delete(firstKey); +} async function cachedComponentize(outputPath, args) { const cacheKey = args.join("+"); - if (cacheKey in CACHE_COMPONENTIZE_OUTPUT) { - await writeFile(outputPath, CACHE_COMPONENTIZE_OUTPUT[cacheKey]); + if (CACHE_COMPONENTIZE_OUTPUT.has(cacheKey)) { + try { + await writeFile(outputPath, CACHE_COMPONENTIZE_OUTPUT.get(cacheKey)); + } catch (error) { + console.warn(`Failed to write cached output: ${error.message}`); + CACHE_COMPONENTIZE_OUTPUT.delete(cacheKey); + } return; } const { stdout, stderr } = await exec( jcoPath, "componentize", ...args, "-o", outputPath, ); strictEqual(stderr, ""); - CACHE_COMPONENTIZE_OUTPUT[cacheKey] = await readFile(outputPath); + if (CACHE_COMPONENTIZE_OUTPUT.size >= MAX_CACHE_SIZE) { + clearOldestCacheEntry(); + } + CACHE_COMPONENTIZE_OUTPUT.set(cacheKey, await readFile(outputPath)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 8c8ed3b and 8558d8e101490a5df901ca762b0841bf7ec3b421.
📒 Files selected for processing (1)
test/cli.js(34 hunks)
🔇 Additional comments (3)
test/cli.js (3)
1-1: LGTM! Clean setup changes.The import statement updates and the addition of
AsyncFunctionconstant are well-structured and follow best practices.Also applies to: 13-13
118-154: LGTM! Well-structured JSPI test with feature detection.The test properly checks for WebAssembly.Suspending support and verifies both sync and async function behaviors.
339-344: LGTM! Clear documentation updates.The comments effectively explain the changes in guest types generation and metadata checks, providing context about upstream dependencies and potential future changes.
Also applies to: 559-565
calvinrp
force-pushed
the
async-pr-first
branch
from
6abb998 to
34d0232
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
test/async.browser.js (2)
57-131: 🛠️ Refactor suggestionImprove error handling for browser and server operations.
The test setup needs better error handling and resource cleanup.
Apply this diff to improve error handling:
test("Transpile async (browser, asyncify)", async () => { + let browser; + let cleanup = async () => {}; + try { const componentName = "async-call"; const { instance, cleanup: componentCleanup, outputDir, } = await setupAsyncTest({ // ... existing setup ... }); + cleanup = componentCleanup; // ... server setup ... // Start a browser to visit the test server - const browser = await puppeteer.launch(); + browser = await puppeteer.launch(); const { page, output: { json }, } = await loadTestPage({ // ... existing setup ... }); deepStrictEqual(json, { responseText: "callAsync" }); - await browser.close(); - await webServerCleanup(); - await componentCleanup(); + } catch (error) { + console.error('Test failed:', error); + throw error; + } finally { + if (browser) { + await browser.close().catch(console.warn); + } + await webServerCleanup().catch(console.warn); + await cleanup().catch(console.warn); } });
133-227: 🛠️ Refactor suggestionImprove error handling and add validation for JSPI browser test.
The JSPI test needs similar error handling improvements and additional JSPI-specific validations.
Apply this diff to improve error handling and add JSPI validation:
if (typeof WebAssembly?.Suspending === "function") { test("Transpile async (browser, JSPI)", async () => { + let browser; + let cleanup = async () => {}; + try { const componentName = "async-call"; const { instance, cleanup: componentCleanup, outputDir, } = await setupAsyncTest({ // ... existing setup ... }); + cleanup = componentCleanup; // ... existing assertions ... // Start a browser to visit the test server - const browser = await puppeteer.launch({ + browser = await puppeteer.launch({ args: [ "--enable-experimental-webassembly-jspi", "--flag-switches-begin", "--enable-features=WebAssemblyExperimentalJSPI", "--flag-switches-end", ], }); + // Verify JSPI is enabled in the browser + const page = await browser.newPage(); + const jspiEnabled = await page.evaluate(() => { + return typeof WebAssembly?.Suspending === 'function'; + }); + ok(jspiEnabled, 'JSPI should be enabled in the browser'); // ... existing test code ... - await browser.close(); - await webServerCleanup(); - await componentCleanup(); + } catch (error) { + console.error('Test failed:', error); + throw error; + } finally { + if (browser) { + await browser.close().catch(console.warn); + } + await webServerCleanup().catch(console.warn); + await cleanup().catch(console.warn); + } }); }
🧹 Nitpick comments (11)
test/helpers.js (2)
99-116: Remove unusedtestFnparameter.The
testFnparameter is documented but never used in the implementation, and no callers provide this parameter.Remove the parameter from both the function signature and JSDoc.
202-202: Simplify error handling with optional chaining.The error check can be simplified using optional chaining.
Apply this diff:
- if (err && err.code && err.code === "ENOENT") { + if (err?.code === "ENOENT") {🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/cmd/transpile.js (2)
17-31: Consider using a configuration file for default async operations.The hardcoded lists of default async imports and exports would be better maintained in a separate configuration file. This would improve maintainability and make it easier to update the lists without modifying the source code.
Consider moving these constants to a configuration file:
-const DEFAULT_ASYNC_IMPORTS = [ - "wasi:io/poll#poll", - "wasi:io/poll#[method]pollable.block", - // ... -]; - -const DEFAULT_ASYNC_EXPORTS = [ - "wasi:cli/run#run", - "wasi:http/incoming-handler#handle", -]; +import { DEFAULT_ASYNC_IMPORTS, DEFAULT_ASYNC_EXPORTS } from '../config/async-defaults.js';
216-224: Improve the TODO comment regarding async WASI shim mapping.The TODO comment on line 216 lacks context and clarity about what needs to be done.
- const maybeAsync = ''; // TODO: !opts.asyncMode || opts.asyncMode === 'sync' ? '' : 'async/'; + // TODO(async-shim): Implement async WASI shim mapping based on asyncMode. + // When asyncMode is enabled and not 'sync', we should use the async version + // of the WASI shims by appending 'async/' to the import path. + const maybeAsync = '';crates/js-component-bindgen/src/ts_bindgen.rs (2)
63-71: Consider using a dedicated type for async configuration.The async mode configuration logic could be encapsulated in a dedicated type.
#[derive(Default)] struct AsyncConfig { imports: HashSet<String>, exports: HashSet<String>, } impl AsyncConfig { fn from_mode(mode: Option<AsyncMode>) -> Self { match mode { None | Some(AsyncMode::Sync) => Default::default(), Some(AsyncMode::JavaScriptPromiseIntegration { imports, exports }) => { Self { imports: imports.into_iter().collect(), exports: exports.into_iter().collect(), } } Some(AsyncMode::Asyncify { imports, exports }) => { Self { imports: imports.into_iter().collect(), exports: exports.into_iter().collect(), } } } } } // Usage: let AsyncConfig { imports: async_imports, exports: async_exports } = AsyncConfig::from_mode(opts.async_mode.clone());
Line range hint
780-914: Add documentation for async function generation.The
ts_funcmethod's async support lacks documentation explaining the behavior.Add documentation:
/// Generates TypeScript function declarations with optional async support. /// /// # Arguments /// /// * `func` - The function to generate TypeScript for /// * `default` - Whether this is a default export /// * `declaration` - Whether to generate a declaration or implementation /// * `is_async` - Whether to generate an async function /// /// # Examples /// /// ```rust /// // Generates: async function foo(): Promise<void> /// ts_func(&func, false, true, true); /// /// // Generates: function foo(): void /// ts_func(&func, false, true, false); /// ``` fn ts_func(&mut self, func: &Function, default: bool, declaration: bool, is_async: bool)crates/js-component-bindgen/src/function_bindgen.rs (1)
1052-1058: Consider using a template for async function calls.The async function call formatting could be improved using a template.
fn format_async_call(&self, callee: &str, args: &str) -> String { if self.is_async { format!("await {}({})", callee, args) } else { format!("{}({})", callee, args) } } // Usage in CallWasm: uwriteln!( self.src, "{};", self.format_async_call(self.callee, &operands.join(", ")) );crates/js-component-bindgen/src/transpile_bindgen.rs (3)
138-142: Consider using a more descriptive type for tracking async functions.Instead of using a tuple of
(String, bool), consider creating a dedicated struct to improve code readability and maintainability.-all_core_exported_funcs: Vec<(String, bool)>, +struct CoreExportedFunc { + name: String, + is_async: bool, +} +all_core_exported_funcs: Vec<CoreExportedFunc>,
1185-1194: Simplify async import name resolution.The nested string manipulation for checking async imports could be refactored into a helper function to improve readability.
+fn is_async_import(name: &str, func_name: &str, async_imports: &HashSet<String>) -> bool { + if async_imports.contains(&format!("{name}#{func_name}")) { + return true; + } + if let Some(i) = name.find('@') { + async_imports.contains(&format!("{}#{func_name}", name.get(0..i).unwrap())) + } else { + false + } +} + -let is_async = self - .async_imports - .contains(&format!("{import_name}#{func_name}")) - || import_name - .find('@') - .map(|i| { - self.async_imports - .contains(&format!("{}#{func_name}", import_name.get(0..i).unwrap())) - }) - .unwrap_or(false); +let is_async = is_async_import(import_name, func_name, &self.async_imports);
2054-2087: Consider caching the async status check results.The async export name resolution is performed multiple times for the same function. Consider caching the results to improve performance.
+use std::collections::HashMap; + +struct AsyncExportCache { + cache: HashMap<(String, String), bool>, +} + +impl AsyncExportCache { + fn new() -> Self { + Self { + cache: HashMap::new(), + } + } + + fn is_async(&mut self, export_name: &str, func_name: &str, async_exports: &HashSet<String>) -> bool { + let key = (export_name.to_string(), func_name.to_string()); + *self.cache.entry(key).or_insert_with(|| { + async_exports.contains(func_name) + || async_exports.contains(&format!("{export_name}#{func_name}")) + || export_name + .find('@') + .map(|i| { + async_exports.contains(&format!( + "{}#{}", + export_name.get(0..i).unwrap(), + func_name + )) + }) + .unwrap_or(false) + }) + } +}src/jco.js (1)
84-88: Consider extracting common async options to reduce duplication.The async options are duplicated between the
transpileandtypescommands. Consider:
- Extracting these options into a shared configuration object
- Creating a helper function to add async options to commands
Example implementation:
// Create a helper function to add async options function addAsyncOptions(command) { return command .addOption(new Option('--async-mode [mode]', 'EXPERIMENTAL: use async imports and exports') .choices(['sync', 'jspi', 'asyncify']) .preset('sync')) .option('--default-async-imports', 'EXPERIMENTAL: default async component imports from WASI interfaces') .option('--default-async-exports', 'EXPERIMENTAL: default async component exports from WASI interfaces') .option('--async-imports <imports...>', 'EXPERIMENTAL: async component imports (examples: "wasi:io/poll@0.2.0#poll", "wasi:io/poll#[method]pollable.block")') .option('--async-exports <exports...>', 'EXPERIMENTAL: async component exports (examples: "wasi:cli/run@#run", "handle")'); } // Usage: program.command('transpile') // ... other options ... .addAsyncOptions() // ... remaining options ... program.command('types') // ... other options ... .addAsyncOptions() // ... remaining options ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6abb9980aad892ddcd5df00212cdf89ddc824d5b and 34d02325428d5a0cc56b5c638346fa32b739b6a8.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/components/async_call.component.wasmis excluded by!**/*.wasm
📒 Files selected for processing (20)
crates/js-component-bindgen-component/src/lib.rs(3 hunks)crates/js-component-bindgen-component/wit/js-component-bindgen.wit(2 hunks)crates/js-component-bindgen/src/function_bindgen.rs(3 hunks)crates/js-component-bindgen/src/intrinsics.rs(8 hunks)crates/js-component-bindgen/src/lib.rs(1 hunks)crates/js-component-bindgen/src/transpile_bindgen.rs(19 hunks)crates/js-component-bindgen/src/ts_bindgen.rs(16 hunks)docs/src/transpiling.md(1 hunks)package.json(1 hunks)src/cmd/opt.js(4 hunks)src/cmd/transpile.js(8 hunks)src/jco.js(2 hunks)test/async.browser.js(1 hunks)test/async.js(1 hunks)test/cli.js(34 hunks)test/fixtures/browser/test-pages/something__test.async.html(1 hunks)test/helpers.js(2 hunks)test/test.js(2 hunks)xtask/src/build/jco.rs(1 hunks)xtask/src/generate/wasi_types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- xtask/src/generate/wasi_types.rs
- test/test.js
- xtask/src/build/jco.rs
- crates/js-component-bindgen/src/lib.rs
- package.json
- crates/js-component-bindgen-component/src/lib.rs
- test/async.js
- test/fixtures/browser/test-pages/something__test.async.html
- src/cmd/opt.js
🧰 Additional context used
🪛 Biome (1.9.4)
test/helpers.js
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
docs/src/transpiling.md
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript * --async-mode [mode]: EXPERIMENTAL: For the component imports...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). * --async-imports <imports...>`: EXPERIMENTAL: Specify the component imp...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. * --async-exports <exports...>`: EXPERIMENTAL: Specify the component exp...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (14)
crates/js-component-bindgen-component/wit/js-component-bindgen.wit (1)
69-71: Well-structured async mode configuration.The async mode configuration is clearly documented and properly integrated into the existing options structure.
docs/src/transpiling.md (1)
56-59: Clear and well-documented experimental features.The documentation clearly explains the new async mode options and properly marks them as experimental.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript *--async-mode [mode]: EXPERIMENTAL: For the component imports...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). *--async-imports <imports...>`: EXPERIMENTAL: Specify the component imp...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. *--async-exports <exports...>`: EXPERIMENTAL: Specify the component exp...(UNLIKELY_OPENING_PUNCTUATION)
test/helpers.js (1)
244-246: Address TODO comments regarding module import debugging.These TODO comments indicate potential issues with module imports that need investigation.
Let's verify the module import functionality:
Would you like me to help investigate and resolve these module import issues?
Also applies to: 253-255
✅ Verification successful
Remove outdated TODO comments and debug code
These TODO comments and associated debug code can be safely removed as the module import functionality is working correctly. The code successfully:
- Creates the module output directory
- Writes the necessary package.json
- Imports the module using the correct file URL format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for module import related issues rg -A 5 "module import not working" --type js rg -A 5 "import.*from.*esModuleSourcePathURL" --type jsLength of output: 805
Script:
#!/bin/bash # Check for active module imports and related code ast-grep --pattern 'import $_ from $_' test/helpers.js echo "---" # Look for moduleOutputDir usage rg -B 5 -A 5 "moduleOutputDir" test/helpers.js echo "---" # Check for dynamic imports ast-grep --pattern 'await import($_)' test/helpers.jsLength of output: 1864
src/cmd/transpile.js (2)
239-247: LGTM! Consistent async mode configuration.The async mode configuration in
transpileComponentfollows the same pattern as the earlier configuration, ensuring consistency throughout the codebase.
79-92: Verify the async mode configuration.The code concatenates default async imports/exports with user-defined ones, but only if
defaultAsyncImportsanddefaultAsyncExportsare true. This could lead to unexpected behavior if the user is unaware of these defaults.Run this script to check for potential conflicts between default and user-defined async operations:
test/cli.js (2)
118-190: LGTM! Comprehensive async mode test coverage.The tests thoroughly validate both JSPI and Asyncify modes, checking synchronous and asynchronous function behavior. The conditional test for JSPI based on
WebAssembly.Suspendingavailability is a good practice.
137-140: Use consistent path handling methods.Similar path handling issues as previously identified in past reviews.
Replace string concatenation with
path.join()orpath.resolve()for better cross-platform compatibility:-await writeFile( - `${outDir}/package.json`, - JSON.stringify({ type: "module" }), -); +await writeFile( + path.join(outDir, "package.json"), + JSON.stringify({ type: "module" }), +);Also applies to: 174-177, 273-276, 294-297, 318-321
crates/js-component-bindgen/src/intrinsics.rs (1)
8-11: LGTM! Clear and well-structured enum variants.The new variants in the
Intrinsicenum are well-named and follow the existing pattern.crates/js-component-bindgen/src/ts_bindgen.rs (1)
36-37: LGTM! Well-structured async support in TsBindgen.The new fields
async_importsandasync_exportsare appropriately added to theTsBindgenstruct.crates/js-component-bindgen/src/function_bindgen.rs (1)
89-89: LGTM! Simple and effective async flag.The
is_asyncfield is a straightforward addition to theFunctionBindgenstruct.crates/js-component-bindgen/src/transpile_bindgen.rs (4)
73-89: Well-structured async mode configuration.The
AsyncModeenum provides a clear and flexible way to configure async behavior:
- Sync mode for synchronous execution
- JSPI mode with configurable imports/exports
- Asyncify mode with configurable imports/exports
271-288: LGTM: Clean implementation of async function wrapping.The code correctly handles both Asyncify and JSPI cases when wrapping core exported functions.
2089-2091: LGTM: Clean async function generation.The code correctly handles the generation of async function declarations based on the function type.
156-168: Consider adding validation for async configuration.The async mode configuration parsing should validate that the imports and exports lists don't contain duplicates or invalid function names.
test/cli.js
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding cache invalidation and size limits.
The caching mechanism for componentization outputs stores everything in memory without any limits or invalidation strategy.
Add cache size limits and invalidation:
// Cache of componentize byte outputs
-const CACHE_COMPONENTIZE_OUTPUT = {};
+const CACHE_COMPONENTIZE_OUTPUT = new Map();
+const MAX_CACHE_SIZE = 100; // Maximum number of entries
+const MAX_CACHE_BYTES = 50 * 1024 * 1024; // Maximum total size (50MB)
+let currentCacheBytes = 0;
+
+function clearCacheIfNeeded(newEntrySize) {
+ if (CACHE_COMPONENTIZE_OUTPUT.size >= MAX_CACHE_SIZE ||
+ currentCacheBytes + newEntrySize > MAX_CACHE_BYTES) {
+ CACHE_COMPONENTIZE_OUTPUT.clear();
+ currentCacheBytes = 0;
+ }
+}
async function cachedComponentize(outputPath, args) {
const cacheKey = args.join("+");
- if (cacheKey in CACHE_COMPONENTIZE_OUTPUT) {
- await writeFile(outputPath, CACHE_COMPONENTIZE_OUTPUT[cacheKey]);
+ if (CACHE_COMPONENTIZE_OUTPUT.has(cacheKey)) {
+ await writeFile(outputPath, CACHE_COMPONENTIZE_OUTPUT.get(cacheKey));
return;
}
const { stdout, stderr } = await exec(
jcoPath,
"componentize",
...args,
"-o",
outputPath,
);
strictEqual(stderr, "");
- CACHE_COMPONENTIZE_OUTPUT[cacheKey] = await readFile(outputPath);
+ const output = await readFile(outputPath);
+ clearCacheIfNeeded(output.length);
+ currentCacheBytes += output.length;
+ CACHE_COMPONENTIZE_OUTPUT.set(cacheKey, output);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Cache of componentize byte outputs | |
| const CACHE_COMPONENTIZE_OUTPUT = {}; | |
| /** | |
| * Small cache for componentizations to save build time by storing componentize | |
| * output in memory | |
| * | |
| * @param {string} outputPath - path to where to write the component | |
| * @param {string[]} args - arguments to be fed to `jco componentize` (*without* "compnentize" or "-o/--output") | |
| */ | |
| async function cachedComponentize(outputPath, args) { | |
| const cacheKey = args.join("+"); | |
| if (cacheKey in CACHE_COMPONENTIZE_OUTPUT) { | |
| await writeFile(outputPath, CACHE_COMPONENTIZE_OUTPUT[cacheKey]); | |
| return; | |
| } | |
| const { stdout, stderr } = await exec( | |
| jcoPath, | |
| "componentize", | |
| ...args, | |
| "-o", | |
| outputPath, | |
| ); | |
| strictEqual(stderr, ""); | |
| CACHE_COMPONENTIZE_OUTPUT[cacheKey] = await readFile(outputPath); | |
| } | |
| // Cache of componentize byte outputs | |
| const CACHE_COMPONENTIZE_OUTPUT = new Map(); | |
| const MAX_CACHE_SIZE = 100; // Maximum number of entries | |
| const MAX_CACHE_BYTES = 50 * 1024 * 1024; // Maximum total size (50MB) | |
| let currentCacheBytes = 0; | |
| function clearCacheIfNeeded(newEntrySize) { | |
| if (CACHE_COMPONENTIZE_OUTPUT.size >= MAX_CACHE_SIZE || | |
| currentCacheBytes + newEntrySize > MAX_CACHE_BYTES) { | |
| CACHE_COMPONENTIZE_OUTPUT.clear(); | |
| currentCacheBytes = 0; | |
| } | |
| } | |
| /** | |
| * Small cache for componentizations to save build time by storing componentize | |
| * output in memory | |
| * | |
| * @param {string} outputPath - path to where to write the component | |
| * @param {string[]} args - arguments to be fed to `jco componentize` (*without* "compnentize" or "-o/--output") | |
| */ | |
| async function cachedComponentize(outputPath, args) { | |
| const cacheKey = args.join("+"); | |
| if (CACHE_COMPONENTIZE_OUTPUT.has(cacheKey)) { | |
| await writeFile(outputPath, CACHE_COMPONENTIZE_OUTPUT.get(cacheKey)); | |
| return; | |
| } | |
| const { stdout, stderr } = await exec( | |
| jcoPath, | |
| "componentize", | |
| ...args, | |
| "-o", | |
| outputPath, | |
| ); | |
| strictEqual(stderr, ""); | |
| const output = await readFile(outputPath); | |
| clearCacheIfNeeded(output.length); | |
| currentCacheBytes += output.length; | |
| CACHE_COMPONENTIZE_OUTPUT.set(cacheKey, output); | |
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for unfinished async operations.
The asyncifyWrapImport function should clean up state when the operation is interrupted.
function asyncifyWrapImport(fn) {
return (...args) => {
if (asyncifyState() === 2) {
asyncifyModules.forEach(({ instance }) => {
instance.exports.asyncify_stop_rewind();
});
const ret = asyncifyResolved;
asyncifyResolved = undefined;
+ asyncifyPromise = undefined;
return ret;
}
asyncifyAssertNoneState();
let value = fn(...args);
+ if (value && typeof value.catch === 'function') {
+ value = value.catch(err => {
+ asyncifyPromise = undefined;
+ asyncifyResolved = undefined;
+ throw err;
+ });
+ }
asyncifyModules.forEach(({ instance, address }) => {
instance.exports.asyncify_start_unwind(address);
});
asyncifyPromise = value;
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Intrinsic::AsyncifyWrapImport => output.push_str(" | |
| function asyncifyWrapImport(fn) { | |
| return (...args) => { | |
| if (asyncifyState() === 2) { | |
| asyncifyModules.forEach(({ instance }) => { | |
| instance.exports.asyncify_stop_rewind(); | |
| }); | |
| const ret = asyncifyResolved; | |
| asyncifyResolved = undefined; | |
| return ret; | |
| } | |
| asyncifyAssertNoneState(); | |
| let value = fn(...args); | |
| asyncifyModules.forEach(({ instance, address }) => { | |
| instance.exports.asyncify_start_unwind(address); | |
| }); | |
| asyncifyPromise = value; | |
| }; | |
| } | |
| "), | |
| Intrinsic::AsyncifyWrapImport => output.push_str(" | |
| function asyncifyWrapImport(fn) { | |
| return (...args) => { | |
| if (asyncifyState() === 2) { | |
| asyncifyModules.forEach(({ instance }) => { | |
| instance.exports.asyncify_stop_rewind(); | |
| }); | |
| const ret = asyncifyResolved; | |
| asyncifyResolved = undefined; | |
| asyncifyPromise = undefined; | |
| return ret; | |
| } | |
| asyncifyAssertNoneState(); | |
| let value = fn(...args); | |
| if (value && typeof value.catch === 'function') { | |
| value = value.catch(err => { | |
| asyncifyPromise = undefined; | |
| asyncifyResolved = undefined; | |
| throw err; | |
| }); | |
| } | |
| asyncifyModules.forEach(({ instance, address }) => { | |
| instance.exports.asyncify_start_unwind(address); | |
| }); | |
| asyncifyPromise = value; | |
| }; | |
| } | |
| "), |
There was a problem hiding this comment.
Add safeguards against concurrent async operations.
The asyncifyWrapExport function doesn't handle concurrent async operations, which could lead to state corruption.
function asyncifyWrapExport(fn) {
+ let isRunning = false;
return async (...args) => {
if (asyncifyModules.length === 0) {
throw new Error(`none of the Wasm modules were processed with wasm-opt asyncify`);
}
+ if (isRunning) {
+ throw new Error('Concurrent async operations are not supported');
+ }
+ isRunning = true;
asyncifyAssertNoneState();
- let result = fn(...args);
- while (asyncifyState() === 1) {
- asyncifyModules.forEach(({ instance }) => {
- instance.exports.asyncify_stop_unwind();
- });
- asyncifyResolved = await asyncifyPromise;
- asyncifyPromise = undefined;
- asyncifyAssertNoneState();
- asyncifyModules.forEach(({ instance, address }) => {
- instance.exports.asyncify_start_rewind(address);
- });
- result = fn(...args);
+ try {
+ let result = fn(...args);
+ while (asyncifyState() === 1) {
+ asyncifyModules.forEach(({ instance }) => {
+ instance.exports.asyncify_stop_unwind();
+ });
+ asyncifyResolved = await asyncifyPromise;
+ asyncifyPromise = undefined;
+ asyncifyAssertNoneState();
+ asyncifyModules.forEach(({ instance, address }) => {
+ instance.exports.asyncify_start_rewind(address);
+ });
+ result = fn(...args);
+ }
+ asyncifyAssertNoneState();
+ return result;
+ } finally {
+ isRunning = false;
}
- asyncifyAssertNoneState();
- return result;
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Intrinsic::AsyncifyWrapExport => output.push_str(" | |
| function asyncifyWrapExport(fn) { | |
| return async (...args) => { | |
| if (asyncifyModules.length === 0) { | |
| throw new Error(`none of the Wasm modules were processed with wasm-opt asyncify`); | |
| } | |
| asyncifyAssertNoneState(); | |
| let result = fn(...args); | |
| while (asyncifyState() === 1) { | |
| asyncifyModules.forEach(({ instance }) => { | |
| instance.exports.asyncify_stop_unwind(); | |
| }); | |
| asyncifyResolved = await asyncifyPromise; | |
| asyncifyPromise = undefined; | |
| asyncifyAssertNoneState(); | |
| asyncifyModules.forEach(({ instance, address }) => { | |
| instance.exports.asyncify_start_rewind(address); | |
| }); | |
| result = fn(...args); | |
| } | |
| asyncifyAssertNoneState(); | |
| return result; | |
| }; | |
| } | |
| "), | |
| Intrinsic::AsyncifyWrapExport => output.push_str(" | |
| function asyncifyWrapExport(fn) { | |
| let isRunning = false; | |
| return async (...args) => { | |
| if (asyncifyModules.length === 0) { | |
| throw new Error(`none of the Wasm modules were processed with wasm-opt asyncify`); | |
| } | |
| if (isRunning) { | |
| throw new Error('Concurrent async operations are not supported'); | |
| } | |
| isRunning = true; | |
| asyncifyAssertNoneState(); | |
| try { | |
| let result = fn(...args); | |
| while (asyncifyState() === 1) { | |
| asyncifyModules.forEach(({ instance }) => { | |
| instance.exports.asyncify_stop_unwind(); | |
| }); | |
| asyncifyResolved = await asyncifyPromise; | |
| asyncifyPromise = undefined; | |
| asyncifyAssertNoneState(); | |
| asyncifyModules.forEach(({ instance, address }) => { | |
| instance.exports.asyncify_start_rewind(address); | |
| }); | |
| result = fn(...args); | |
| } | |
| asyncifyAssertNoneState(); | |
| return result; | |
| } finally { | |
| isRunning = false; | |
| } | |
| }; | |
| } | |
| "), |
There was a problem hiding this comment.
Add error handling for memory allocation failures.
The memory allocation in asyncifyInstantiate lacks error handling for allocation failures.
async function asyncifyInstantiate(module, imports) {
const instance = await instantiateCore(module, imports);
const memory = instance.exports.memory || (imports && imports.env && imports.env.memory);
const realloc = instance.exports.cabi_realloc || instance.exports.cabi_export_realloc;
if (instance.exports.asyncify_get_state && memory) {
let address;
if (realloc) {
- address = realloc(0, 0, 4, 1024);
+ try {
+ address = realloc(0, 0, 4, 1024);
+ if (address === 0) throw new Error('Memory allocation failed');
+ } catch (e) {
+ throw new Error(`Failed to allocate memory for asyncify: ${e.message}`);
+ }
new Int32Array(memory.buffer, address).set([address + 8, address + 1024]);
} else {
address = 16;
new Int32Array(memory.buffer, address).set([address + 8, address + 1024]);
}
asyncifyModules.push({ instance, memory, address });
}
return instance;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Intrinsic::AsyncifyAsyncInstantiate => output.push_str(" | |
| const asyncifyModules = []; | |
| let asyncifyPromise; | |
| let asyncifyResolved; | |
| async function asyncifyInstantiate(module, imports) { | |
| const instance = await instantiateCore(module, imports); | |
| const memory = instance.exports.memory || (imports && imports.env && imports.env.memory); | |
| const realloc = instance.exports.cabi_realloc || instance.exports.cabi_export_realloc; | |
| if (instance.exports.asyncify_get_state && memory) { | |
| let address; | |
| if (realloc) { | |
| address = realloc(0, 0, 4, 1024); | |
| new Int32Array(memory.buffer, address).set([address + 8, address + 1024]); | |
| } else { | |
| address = 16; | |
| new Int32Array(memory.buffer, address).set([address + 8, address + 1024]); | |
| } | |
| asyncifyModules.push({ instance, memory, address }); | |
| } | |
| return instance; | |
| } | |
| function asyncifyState() { | |
| return asyncifyModules[0]?.instance.exports.asyncify_get_state(); | |
| } | |
| function asyncifyAssertNoneState() { | |
| let state = asyncifyState(); | |
| if (state !== 0) { | |
| throw new Error(`reentrancy not supported, expected asyncify state '0' but found '${state}'`); | |
| } | |
| } | |
| "), | |
| Intrinsic::AsyncifyAsyncInstantiate => output.push_str(" | |
| const asyncifyModules = []; | |
| let asyncifyPromise; | |
| let asyncifyResolved; | |
| async function asyncifyInstantiate(module, imports) { | |
| const instance = await instantiateCore(module, imports); | |
| const memory = instance.exports.memory || (imports && imports.env && imports.env.memory); | |
| const realloc = instance.exports.cabi_realloc || instance.exports.cabi_export_realloc; | |
| if (instance.exports.asyncify_get_state && memory) { | |
| let address; | |
| if (realloc) { | |
| try { | |
| address = realloc(0, 0, 4, 1024); | |
| if (address === 0) throw new Error('Memory allocation failed'); | |
| } catch (e) { | |
| throw new Error(`Failed to allocate memory for asyncify: ${e.message}`); | |
| } | |
| new Int32Array(memory.buffer, address).set([address + 8, address + 1024]); | |
| } else { | |
| address = 16; | |
| new Int32Array(memory.buffer, address).set([address + 8, address + 1024]); | |
| } | |
| asyncifyModules.push({ instance, memory, address }); | |
| } | |
| return instance; | |
| } | |
| function asyncifyState() { | |
| return asyncifyModules[0]?.instance.exports.asyncify_get_state(); | |
| } | |
| function asyncifyAssertNoneState() { | |
| let state = asyncifyState(); | |
| if (state !== 0) { | |
| throw new Error(`reentrancy not supported, expected asyncify state '0' but found '${state}'`); | |
| } | |
| } | |
| "), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for async method calls.
The async method calls in CallInterface lack proper error handling.
fn format_async_method_call(&self, obj: &str, method: &str, args: &str) -> String {
let call = if self.callee_resource_dynamic {
format!("{}.{}({})", obj, method, args)
} else {
format!("{}({})", method, args)
};
if self.is_async {
format!(
"try {{ await {} }} catch (e) {{ throw new Error(`Failed to call {}: ${{e.message}}`) }}",
call, method
)
} else {
call
}
}
// Usage in CallInterface:
let call = self.format_async_method_call(
&operands[0],
self.callee,
&operands[1..].join(", ")
);
src/jco.js
Outdated
There was a problem hiding this comment.
💡 Codebase verification
Add validation for async options and enhance documentation
The documentation and tests confirm the intended usage of async features, but there are still gaps to address:
- Implement validation to ensure:
--async-modeis set when using any async-related option- Proper handling of conflicts between
--default-async-*and specific--async-*options
- Update documentation in
docs/src/transpiling.mdto:- Document the
--default-async-importsand--default-async-exportsoptions - Explain the interaction between default and specific async options
- Document the
🔗 Analysis chain
Consider adding validation for async-related options.
The new experimental async options look good, but consider adding validation to:
- Ensure
--async-modeis set when using other async options - Prevent conflicts between default async flags and specific async lists
- Add links to documentation in the help text for these experimental features
Let's verify the documentation for these experimental features:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if documentation exists for the experimental async features
# Look for documentation files
fd -t f -e md . | xargs rg -l "async-mode|jspi|asyncify"
# Look for examples or tests
fd -t f -e js -e ts . | xargs rg -l "async-mode.*jspi|async-mode.*asyncify"
Length of output: 183
Script:
#!/bin/bash
# Check documentation content
echo "=== Documentation content ==="
rg -A 5 "async-mode|jspi|asyncify" docs/src/transpiling.md
echo -e "\n=== Test coverage ==="
rg -A 5 "async-mode.*jspi|async-mode.*asyncify" test/cli.js
echo -e "\n=== Validation implementation ==="
ast-grep --pattern 'function validateAsyncOptions($$$) { $$$ }' src/jco.js
Length of output: 1558
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cli.js (2)
118-190: Consider reducing code duplication in async tests.The JSPI and Asyncify tests share significant code duplication. Consider extracting the common test logic into a helper function.
+async function testAsyncMode(asyncMode) { + const name = "async_call"; + const { stderr } = await exec( + jcoPath, + "transpile", + `test/fixtures/components/${name}.component.wasm`, + `--name=${name}`, + "--valid-lifting-optimization", + "--tla-compat", + "--instantiation=async", + "--base64-cutoff=0", + `--async-mode=${asyncMode}`, + "--async-imports=something:test/test-interface#call-async", + "--async-exports=run-async", + "-o", + outDir, + ); + strictEqual(stderr, ""); + await writeFile( + `${outDir}/package.json`, + JSON.stringify({ type: "module" }), + ); + const m = await import(`${pathToFileURL(outDir)}/${name}.js`); + const inst = await m.instantiate(undefined, { + "something:test/test-interface": { + callAsync: async () => "called async", + callSync: () => "called sync", + }, + }); + strictEqual(inst.runSync instanceof AsyncFunction, false); + strictEqual(inst.runAsync instanceof AsyncFunction, true); + + strictEqual(inst.runSync(), "called sync"); + strictEqual(await inst.runAsync(), "called async"); +} + if (typeof WebAssembly.Suspending === "function") { - test("Transpile with Async Mode for JSPI", async () => { - const name = "async_call"; - // ... existing JSPI test code ... - }); + test("Transpile with Async Mode for JSPI", () => testAsyncMode("jspi")); } -test("Transpile with Async Mode for Asyncify", async () => { - const name = "async_call"; - // ... existing Asyncify test code ... -}); +test("Transpile with Async Mode for Asyncify", () => testAsyncMode("asyncify"));
657-663: Enhance function documentation.Consider improving the JSDoc comments with:
- Return type documentation
- More detailed parameter descriptions
- Example usage
/** * Small cache for componentizations to save build time by storing componentize * output in memory * * @param {string} outputPath - path to where to write the component * @param {string[]} args - arguments to be fed to `jco componentize` (*without* "compnentize" or "-o/--output") + * @returns {Promise<void>} - Resolves when the component has been written + * @example + * // Example usage: + * await cachedComponentize('output.wasm', ['input.js', '-d', 'all', '--aot']); */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 34d02325428d5a0cc56b5c638346fa32b739b6a8 and 627b038291672a54122efa03e0021a3fbbd1c543.
📒 Files selected for processing (1)
test/cli.js(34 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Jco Build
🔇 Additional comments (3)
test/cli.js (3)
1-1: LGTM! Good setup for async functionality testing.The addition of the
AsyncFunctionconstant and thejoinimport prepares the groundwork for the new async tests.Also applies to: 13-13
56-58: Use consistent path handling methods.String concatenation is still being used for paths instead of
path.join()orpath.resolve().Also applies to: 137-140, 174-177
654-681: Implement cache size limits and invalidation strategy.The current implementation stores everything in memory without any limits or invalidation strategy.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/cli.js (2)
118-154: Consider reducing code duplication in async tests.While the implementation is correct and thoroughly tests both JSPI and Asyncify modes, there's significant code duplication between the two test cases.
Consider extracting the common test logic into a helper function:
+async function testAsyncMode(asyncMode) { + const name = "async_call"; + const { stderr } = await exec( + jcoPath, + "transpile", + `test/fixtures/components/${name}.component.wasm`, + `--name=${name}`, + "--valid-lifting-optimization", + "--tla-compat", + "--instantiation=async", + "--base64-cutoff=0", + `--async-mode=${asyncMode}`, + "--async-imports=something:test/test-interface#call-async", + "--async-exports=run-async", + "-o", + outDir, + ); + strictEqual(stderr, ""); + await writeFile( + `${outDir}/package.json`, + JSON.stringify({ type: "module" }), + ); + const m = await import(`${pathToFileURL(outDir)}/${name}.js`); + const inst = await m.instantiate(undefined, { + "something:test/test-interface": { + callAsync: async () => "called async", + callSync: () => "called sync", + }, + }); + strictEqual(inst.runSync instanceof AsyncFunction, false); + strictEqual(inst.runAsync instanceof AsyncFunction, true); + + strictEqual(inst.runSync(), "called sync"); + strictEqual(await inst.runAsync(), "called async"); +} + +if (typeof WebAssembly.Suspending === "function") { + test("Transpile with Async Mode for JSPI", () => testAsyncMode("jspi")); +} + +test("Transpile with Async Mode for Asyncify", () => testAsyncMode("asyncify"));Also applies to: 156-190
118-190: Consider adding more test cases for async functionality.The current tests cover the happy path, but consider adding tests for:
- Error cases (invalid async modes, missing imports)
- Performance impact of different async modes
- Memory usage patterns with Asyncify
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 627b038291672a54122efa03e0021a3fbbd1c543 and b70dd6b2c98e8d08e5e7ee5518d496dac2647a64.
📒 Files selected for processing (1)
test/cli.js(34 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Jco Test Suite (macos-latest, latest)
- GitHub Check: Jco Node.js WASI Conformance Tests (macos-latest, latest)
- GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, latest)
- GitHub Check: Jco Test Suite (ubuntu-22.04, latest)
- GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, 20.x)
- GitHub Check: Jco Test Suite (ubuntu-22.04, 20.x)
- GitHub Check: Jco Test Suite (ubuntu-22.04, 18.x)
- GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, 18.x)
- GitHub Check: Jco Deno WASI Conformance Tests
🔇 Additional comments (3)
test/cli.js (3)
1-1: LGTM! Import changes and AsyncFunction setup look good.The addition of
joinimport andAsyncFunctionconstant properly supports the new async functionality and path handling improvements.Also applies to: 13-13
137-140: Use consistent path handling methods.String concatenation is still being used for paths. As mentioned in the previous review, use
path.join()for better cross-platform compatibility.Also applies to: 174-177
654-681: Consider adding cache invalidation and size limits.The caching mechanism stores everything in memory without any limits or invalidation strategy.
* fix(tests): guest types check Signed-off-by: Victor Adossi <vadossi@cosmonic.com> * fix: re-comment check from upstream jco Signed-off-by: Victor Adossi <vadossi@cosmonic.com> --------- Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
Signed-off-by: Victor Adossi <vadossi@cosmonic.com>
calvinrp
force-pushed
the
async-pr-first
branch
from
1e2b46e to
4027d11
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
test/async.browser.js (2)
57-131:⚠️ Potential issueAdd robust error handling for browser and server operations.
The test needs better cleanup handling in error cases.
test("Transpile async (browser, asyncify)", async () => { + let browser; + let cleanup = async () => {}; + try { const componentName = "async-call"; const { instance, cleanup: componentCleanup, outputDir, } = await setupAsyncTest({ // ... existing setup ... }); + cleanup = componentCleanup; // ... server setup ... // Start a browser to visit the test server - const browser = await puppeteer.launch(); + browser = await puppeteer.launch(); // ... test code ... - await browser.close(); - await webServerCleanup(); - await componentCleanup(); + } catch (error) { + console.error('Test failed:', error); + throw error; + } finally { + if (browser) { + await browser.close().catch(console.warn); + } + await webServerCleanup().catch(console.warn); + await cleanup().catch(console.warn); } });
133-227:⚠️ Potential issueImprove error handling and add validation for JSPI browser test.
Similar error handling improvements needed for the JSPI test.
if (typeof WebAssembly?.Suspending === "function") { test("Transpile async (browser, JSPI)", async () => { + let browser; + let cleanup = async () => {}; + try { const componentName = "async-call"; const { instance, cleanup: componentCleanup, outputDir, } = await setupAsyncTest({ // ... existing setup ... }); + cleanup = componentCleanup; // ... existing assertions ... // Start a browser to visit the test server - const browser = await puppeteer.launch({ + browser = await puppeteer.launch({ args: [ "--enable-experimental-webassembly-jspi", "--flag-switches-begin", "--enable-features=WebAssemblyExperimentalJSPI", "--flag-switches-end", ], }); + // Verify JSPI is enabled in the browser + const page = await browser.newPage(); + const jspiEnabled = await page.evaluate(() => { + return typeof WebAssembly?.Suspending === 'function'; + }); + ok(jspiEnabled, 'JSPI should be enabled in the browser'); // ... existing test code ... - await browser.close(); - await webServerCleanup(); - await componentCleanup(); + } catch (error) { + console.error('Test failed:', error); + throw error; + } finally { + if (browser) { + await browser.close().catch(console.warn); + } + await webServerCleanup().catch(console.warn); + await cleanup().catch(console.warn); + } }); }test/helpers.js (1)
99-279:⚠️ Potential issueRemove unused
testFnparameter.The
testFnparameter is documented but never used in the implementation.- * @param {function} args.testFn - Arguments for running the async test🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/cli.js (1)
654-681: 🛠️ Refactor suggestionConsider adding cache invalidation and size limits.
The caching mechanism for componentization outputs stores everything in memory without any limits or invalidation strategy.
crates/js-component-bindgen/src/function_bindgen.rs (1)
1076-1089: 🛠️ Refactor suggestionAdd error handling for async method calls.
The async method calls in CallInterface lack proper error handling.
🧹 Nitpick comments (12)
crates/js-component-bindgen-component/wit/js-component-bindgen.wit (1)
117-119: Maintain consistent documentation style.The
async-modefield intype-generation-optionsshould have the same documentation comment style as ingenerate-optionsfor consistency.+ /// Configure whether to use `async` imports or exports with + /// JavaScript Promise Integration (JSPI) or Asyncify. async-mode: option<async-mode>,src/cmd/opt.js (2)
89-90: Consider extracting default optimization arguments.The default optimization arguments could be extracted into a constant for better maintainability and reusability.
+const DEFAULT_OPT_ARGS = ['-Oz', '--low-memory-unused', '--enable-bulk-memory', '--strip-debug']; + - const args = opts?.optArgs ? [...opts.optArgs] : ['-Oz', '--low-memory-unused', '--enable-bulk-memory', '--strip-debug']; + const args = opts?.optArgs ? [...opts.optArgs] : [...DEFAULT_OPT_ARGS]; if (opts?.asyncMode === 'asyncify') args.push('--asyncify');
Line range hint
101-145: Robust size calculation and output handling.The implementation correctly handles potential size increases from asyncify and includes a safety buffer. The truncation of unused space is a good optimization.
However, consider adding a debug log for cases where the size increases significantly.
const sizeChange = optimizedModulesTotalSize - previousModulesTotalSize; + if (sizeChange > previousModulesTotalSize * 0.5) { + console.warn(`Significant size increase after optimization: ${((sizeChange / previousModulesTotalSize) * 100).toFixed(2)}%`); + }docs/src/transpiling.md (1)
56-58: Enhance documentation for experimental features.While the documentation clearly marks these features as experimental, consider adding:
- Examples of usage for each option
- Common pitfalls or limitations
- Compatibility requirements (e.g., browser/Node.js version requirements)
* `--async-mode [mode]`: EXPERIMENTAL: For the component imports and exports, functions and methods on resources can be specified as `async`. The two options are `jspi` (JavaScript Promise Integration) and `asyncify` (Binaryen's `wasm-opt --asyncify`). + +Example usage: +```bash +# Using JSPI +jco transpile component.wasm --async-mode jspi --async-imports "my:pkg/interface#async-fn" --async-exports "exported-async-fn" + +# Using Asyncify +jco transpile component.wasm --async-mode asyncify --async-imports "my:pkg/interface#async-fn" --async-exports "exported-async-fn" +``` + +Requirements: +- JSPI requires Node.js >=23 with `--experimental-wasm-jspi` flag +- Asyncify requires Binaryen's wasm-opt + +Note: Asyncify may increase the size of the WebAssembly module.🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript *--async-mode [mode]: EXPERIMENTAL: For the component imports...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). *--async-imports <imports...>`: EXPERIMENTAL: Specify the component imp...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. *--async-exports <exports...>`: EXPERIMENTAL: Specify the component exp...(UNLIKELY_OPENING_PUNCTUATION)
test/helpers.js (1)
482-556: Add rate limiting to web server.Consider adding basic rate limiting to prevent potential DoS attacks during testing.
+const rateLimit = { + windowMs: 1000, // 1 second + maxRequests: 50, + clients: new Map() +}; + export async function startTestWebServer(args) { // ... existing code ... const server = createHttpServer(async (req, res) => { + const clientIp = req.socket.remoteAddress; + const now = Date.now(); + + if (!rateLimit.clients.has(clientIp)) { + rateLimit.clients.set(clientIp, { count: 1, windowStart: now }); + } else { + const client = rateLimit.clients.get(clientIp); + if (now - client.windowStart > rateLimit.windowMs) { + client.count = 1; + client.windowStart = now; + } else if (client.count >= rateLimit.maxRequests) { + res.writeHead(429); + res.end('Too Many Requests'); + return; + } else { + client.count++; + } + } + // ... existing request handling ... });src/cmd/transpile.js (2)
79-92: Consider adding validation for async mode configuration.The async mode configuration logic could benefit from validation to ensure the provided mode is valid and the imports/exports lists contain valid entries.
+ const validAsyncModes = ['sync', 'jspi', 'asyncify']; + if (opts.asyncMode && !validAsyncModes.includes(opts.asyncMode)) { + throw new Error(`Invalid async mode "${opts.asyncMode}". Valid modes are: ${validAsyncModes.join(', ')}`); + } + if (opts.defaultAsyncImports) opts.asyncImports = DEFAULT_ASYNC_IMPORTS.concat(opts.asyncImports || []); if (opts.defaultAsyncExports) opts.asyncExports = DEFAULT_ASYNC_EXPORTS.concat(opts.asyncExports || []);
205-210: Add JSDoc comment for preoptimized and asyncify condition.The comment explains an important optimization condition but could be more detailed with a JSDoc format.
- // We must perform optimization if it's requested, or if the component has *not* - // already been built with already optimized code before wizer init, if using asyncify. - // - // Preoptimized is generally used internally (users cannot pass it in as a CLI option), - // but they may pass in asyncMode + /** + * Optimization is performed in two cases: + * 1. When explicitly requested via opts.optimize + * 2. When using asyncify mode and the component hasn't been preoptimized + * + * Note: preoptimized is an internal flag and cannot be set via CLI, + * but asyncMode can be configured by users + */test/cli.js (1)
118-154: Add test coverage for error cases in JSPI async mode.The JSPI async mode tests only cover the happy path. Consider adding tests for error handling and edge cases.
+ test("Transpile with Async Mode for JSPI - Error Handling", async () => { + const name = "async_call"; + const { stderr } = await exec( + jcoPath, + "transpile", + `test/fixtures/components/${name}.component.wasm`, + `--name=${name}`, + "--async-mode=jspi", + "--async-imports=something:test/test-interface#call-async", + "-o", + outDir, + ); + strictEqual(stderr, ""); + const m = await import(`${pathToFileURL(outDir)}/${name}.js`); + const inst = await m.instantiate(undefined, { + "something:test/test-interface": { + callAsync: async () => { throw new Error("Async error"); }, + }, + }); + await assert.rejects( + () => inst.runAsync(), + /Async error/ + ); + });crates/js-component-bindgen/src/ts_bindgen.rs (1)
Line range hint
780-914: Consider extracting async-related logic into a separate method.The ts_func method has grown complex with the addition of async handling. Consider extracting the async-specific logic into a separate method for better maintainability.
impl<'a> TsInterface<'a> { fn format_async_prefix(&self, is_async: bool) -> &'static str { if is_async { "async " } else { "" } } fn ts_func(&mut self, func: &Function, default: bool, declaration: bool, is_async: bool) { // ... existing setup code ... let maybe_async = self.format_async_prefix(is_async); // ... rest of the method ... } }crates/js-component-bindgen/src/transpile_bindgen.rs (3)
137-142: Consider using a more descriptive name forall_core_exported_funcs.The tuple storing function name and async status could be more clearly named to indicate its purpose, e.g.,
core_exports_async_status.- all_core_exported_funcs: Vec<(String, bool)>, + core_exports_async_status: Vec<(String, bool)>,
271-288: Consider extracting async wrapper logic into a separate function.The core exported functions wrapping logic could be moved to a dedicated function to improve readability and maintainability.
+ fn wrap_core_export(&mut self, core_export_fn: &str, local_name: &str, is_async: bool) { + if is_async { + let async_wrap_fn = if self.use_asyncify { + &self.intrinsic(Intrinsic::AsyncifyWrapExport) + } else { + "WebAssembly.promising" + }; + uwriteln!( + core_exported_funcs, + "{local_name} = {async_wrap_fn}({core_export_fn});", + ); + } else { + uwriteln!(core_exported_funcs, "{local_name} = {core_export_fn};",); + } + }
1096-1101: Ensure consistent instantiation handling between sync and async modes.The instantiation logic for sync and async modes is very similar but duplicated. Consider extracting the common logic to reduce duplication.
+ fn get_instantiate_intrinsic(&self, is_sync: bool) -> &str { + if self.use_asyncify { + if is_sync { + self.gen.intrinsic(Intrinsic::AsyncifySyncInstantiate) + } else { + self.gen.intrinsic(Intrinsic::AsyncifyAsyncInstantiate) + } + } else { + self.gen.intrinsic(Intrinsic::InstantiateCore) + } + }Also applies to: 1109-1114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between b70dd6b2c98e8d08e5e7ee5518d496dac2647a64 and 4027d11.
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/components/async_call.component.wasmis excluded by!**/*.wasm
📒 Files selected for processing (20)
crates/js-component-bindgen-component/src/lib.rs(3 hunks)crates/js-component-bindgen-component/wit/js-component-bindgen.wit(2 hunks)crates/js-component-bindgen/src/function_bindgen.rs(3 hunks)crates/js-component-bindgen/src/intrinsics.rs(8 hunks)crates/js-component-bindgen/src/lib.rs(1 hunks)crates/js-component-bindgen/src/transpile_bindgen.rs(19 hunks)crates/js-component-bindgen/src/ts_bindgen.rs(16 hunks)docs/src/transpiling.md(1 hunks)package.json(1 hunks)src/cmd/opt.js(4 hunks)src/cmd/transpile.js(8 hunks)src/jco.js(2 hunks)test/async.browser.js(1 hunks)test/async.js(1 hunks)test/cli.js(34 hunks)test/fixtures/browser/test-pages/something__test.async.html(1 hunks)test/helpers.js(2 hunks)test/test.js(2 hunks)xtask/src/build/jco.rs(1 hunks)xtask/src/generate/wasi_types.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- xtask/src/build/jco.rs
- test/test.js
- package.json
- xtask/src/generate/wasi_types.rs
- crates/js-component-bindgen/src/lib.rs
- test/fixtures/browser/test-pages/something__test.async.html
- test/async.js
- crates/js-component-bindgen-component/src/lib.rs
- src/jco.js
- crates/js-component-bindgen/src/intrinsics.rs
🧰 Additional context used
🪛 LanguageTool
docs/src/transpiling.md
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript * --async-mode [mode]: EXPERIMENTAL: For the component imports...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). * --async-imports <imports...>`: EXPERIMENTAL: Specify the component imp...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. * --async-exports <exports...>`: EXPERIMENTAL: Specify the component exp...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Biome (1.9.4)
test/helpers.js
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Jco Build
🔇 Additional comments (12)
crates/js-component-bindgen-component/wit/js-component-bindgen.wit (2)
69-85: Well-structured async mode configuration.The new
async-modevariant andasync-imports-exportsrecord provide a clear and flexible way to configure asynchronous behavior. The documentation comments effectively explain each variant's purpose.
61-72: Consider adding validation constraints for async-mode configuration.While the implementation looks good, consider adding documentation about any constraints or interactions between
async-modeand other options (e.g.,instantiationmode).src/cmd/opt.js (1)
65-68: Well-documented function parameters.The JSDoc clearly documents the function parameters and return type. The optional parameters are properly marked.
test/async.browser.js (1)
1-56: Well-structured test setup with comprehensive imports.The test setup includes all necessary imports and helper functions. The
AsyncFunctioncheck is a good way to verify function types.test/helpers.js (2)
36-55: Well-implemented debug logging.The logging implementation is clean and flexible, with good type checking and formatting.
202-202: Apply optional chaining to checkerr.code.Simplify the error code check using optional chaining.
- if (err && err.code && err.code === "ENOENT") { + if (err?.code === "ENOENT") {🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/cmd/transpile.js (2)
17-31: LGTM! Well-structured default async configurations.The default async imports and exports are well organized and follow a consistent pattern. The constants provide a good centralized way to manage default async behaviors.
216-224: Verify the TODO comment for async shim paths.There's a TODO comment about async shim paths that needs to be addressed.
crates/js-component-bindgen/src/ts_bindgen.rs (1)
35-37: LGTM! Clean addition of async support to TsBindgen.The async_imports and async_exports fields are well integrated into the TsBindgen struct.
crates/js-component-bindgen/src/function_bindgen.rs (1)
89-89: LGTM! Clean addition of async flag.The is_async field is a clean addition to the FunctionBindgen struct.
crates/js-component-bindgen/src/transpile_bindgen.rs (2)
73-89: Well-structured enum for async mode configuration.The
AsyncModeenum provides a clear and flexible way to configure async behavior with distinct options for JSPI and Asyncify. The design allows for granular control over which imports and exports should be async.
156-168: Verify error handling for async mode initialization.The async mode initialization doesn't explicitly handle potential edge cases where imports/exports lists contain invalid function names.
Run this script to check for function name validation:
| /** | ||
| * Load a browser page, usually triggering test output that is written | ||
| * to the HTML body of the page | ||
| * | ||
| * @param {object} args | ||
| * @param {object} args.browser - Puppeteer browser instance | ||
| * @param {object} [args.path] - Path to the HTML file to use, with root at `test` (ex. `test/browser.html` would be just `browser.html`) | ||
| * @param {string} args.hash - Hash at which to perform tests (used to identify specific tests) | ||
| */ | ||
| export async function loadTestPage(args) { | ||
| const { browser, hash } = args; | ||
| if (!browser) { | ||
| throw new Error("missing puppeteer instance browser object"); | ||
| } | ||
| if (!hash) { | ||
| throw new Error("missing hash for browser page"); | ||
| } | ||
|
|
||
| const page = await browser.newPage(); | ||
|
|
||
| // Pass along all output to test | ||
| if (env.TEST_DEBUG) { | ||
| page | ||
| .on("console", (message) => | ||
| log( | ||
| `[browser] ${message.type().substr(0, 3).toUpperCase()} ${message.text()}`, | ||
| ), | ||
| ) | ||
| .on("pageerror", ({ message }) => log(`[browser] ${message}`)) | ||
| .on("response", (response) => | ||
| log(`[browser] ${response.status()} ${response.url()}`), | ||
| ) | ||
| .on("requestfailed", (request) => | ||
| log(`[browser] ${request.failure().errorText} ${request.url()}`), | ||
| ); | ||
| } | ||
|
|
||
| const path = args.path ? args.path : "test/browser.html"; | ||
| const serverPort = args.serverPort ? args.serverPort : 8080; | ||
|
|
||
| const hashURL = `http://localhost:${serverPort}/${path}#${hash}`; | ||
| log(`[browser] attempting to navigate to [${hashURL}]`); | ||
| const hashTest = await page.goto(hashURL); | ||
| ok(hashTest.ok(), `navigated to URL [${hashURL}]`); | ||
|
|
||
| const body = await page.locator("body").waitHandle(); | ||
|
|
||
| let bodyHTML = await body.evaluate((el) => el.innerHTML); | ||
| // If the body HTML uses "Running" to show state, wait until it changes | ||
| if (bodyHTML == "<h1>Running</h1>") { | ||
| while (bodyHTML === "<h1>Running</h1>") { | ||
| bodyHTML = await body.evaluate((el) => el.innerHTML); | ||
| } | ||
| } | ||
|
|
||
| // Attempt to parse the HTML body content as JSON | ||
| const raw = bodyHTML; | ||
| let testOutputJSON; | ||
| try { | ||
| testOutputJSON = JSON.parse(raw); | ||
| } catch (err) { | ||
| log(`failed to parse JSON for body HTML: ${err}`); | ||
| } | ||
|
|
||
| return { | ||
| page, | ||
| body, | ||
| output: { | ||
| raw, | ||
| json: testOutputJSON, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling for page loading.
The page loading logic should include a timeout to prevent hanging tests.
export async function loadTestPage(args) {
+ const timeout = args.timeout || 30000; // 30 seconds default timeout
const { browser, hash } = args;
// ... existing code ...
- const hashTest = await page.goto(hashURL);
+ const hashTest = await Promise.race([
+ page.goto(hashURL),
+ new Promise((_, reject) =>
+ setTimeout(() => reject(new Error(`Timeout loading page: ${hashURL}`)), timeout)
+ )
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Load a browser page, usually triggering test output that is written | |
| * to the HTML body of the page | |
| * | |
| * @param {object} args | |
| * @param {object} args.browser - Puppeteer browser instance | |
| * @param {object} [args.path] - Path to the HTML file to use, with root at `test` (ex. `test/browser.html` would be just `browser.html`) | |
| * @param {string} args.hash - Hash at which to perform tests (used to identify specific tests) | |
| */ | |
| export async function loadTestPage(args) { | |
| const { browser, hash } = args; | |
| if (!browser) { | |
| throw new Error("missing puppeteer instance browser object"); | |
| } | |
| if (!hash) { | |
| throw new Error("missing hash for browser page"); | |
| } | |
| const page = await browser.newPage(); | |
| // Pass along all output to test | |
| if (env.TEST_DEBUG) { | |
| page | |
| .on("console", (message) => | |
| log( | |
| `[browser] ${message.type().substr(0, 3).toUpperCase()} ${message.text()}`, | |
| ), | |
| ) | |
| .on("pageerror", ({ message }) => log(`[browser] ${message}`)) | |
| .on("response", (response) => | |
| log(`[browser] ${response.status()} ${response.url()}`), | |
| ) | |
| .on("requestfailed", (request) => | |
| log(`[browser] ${request.failure().errorText} ${request.url()}`), | |
| ); | |
| } | |
| const path = args.path ? args.path : "test/browser.html"; | |
| const serverPort = args.serverPort ? args.serverPort : 8080; | |
| const hashURL = `http://localhost:${serverPort}/${path}#${hash}`; | |
| log(`[browser] attempting to navigate to [${hashURL}]`); | |
| const hashTest = await page.goto(hashURL); | |
| ok(hashTest.ok(), `navigated to URL [${hashURL}]`); | |
| const body = await page.locator("body").waitHandle(); | |
| let bodyHTML = await body.evaluate((el) => el.innerHTML); | |
| // If the body HTML uses "Running" to show state, wait until it changes | |
| if (bodyHTML == "<h1>Running</h1>") { | |
| while (bodyHTML === "<h1>Running</h1>") { | |
| bodyHTML = await body.evaluate((el) => el.innerHTML); | |
| } | |
| } | |
| // Attempt to parse the HTML body content as JSON | |
| const raw = bodyHTML; | |
| let testOutputJSON; | |
| try { | |
| testOutputJSON = JSON.parse(raw); | |
| } catch (err) { | |
| log(`failed to parse JSON for body HTML: ${err}`); | |
| } | |
| return { | |
| page, | |
| body, | |
| output: { | |
| raw, | |
| json: testOutputJSON, | |
| }, | |
| }; | |
| } | |
| /** | |
| * Load a browser page, usually triggering test output that is written | |
| * to the HTML body of the page | |
| * | |
| * @param {object} args | |
| * @param {object} args.browser - Puppeteer browser instance | |
| * @param {object} [args.path] - Path to the HTML file to use, with root at `test` (ex. `test/browser.html` would be just `browser.html`) | |
| * @param {string} args.hash - Hash at which to perform tests (used to identify specific tests) | |
| */ | |
| export async function loadTestPage(args) { | |
| const timeout = args.timeout || 30000; // 30 seconds default timeout | |
| const { browser, hash } = args; | |
| if (!browser) { | |
| throw new Error("missing puppeteer instance browser object"); | |
| } | |
| if (!hash) { | |
| throw new Error("missing hash for browser page"); | |
| } | |
| const page = await browser.newPage(); | |
| // Pass along all output to test | |
| if (env.TEST_DEBUG) { | |
| page | |
| .on("console", (message) => | |
| log( | |
| `[browser] ${message.type().substr(0, 3).toUpperCase()} ${message.text()}`, | |
| ), | |
| ) | |
| .on("pageerror", ({ message }) => log(`[browser] ${message}`)) | |
| .on("response", (response) => | |
| log(`[browser] ${response.status()} ${response.url()}`), | |
| ) | |
| .on("requestfailed", (request) => | |
| log(`[browser] ${request.failure().errorText} ${request.url()}`), | |
| ); | |
| } | |
| const path = args.path ? args.path : "test/browser.html"; | |
| const serverPort = args.serverPort ? args.serverPort : 8080; | |
| const hashURL = `http://localhost:${serverPort}/${path}#${hash}`; | |
| log(`[browser] attempting to navigate to [${hashURL}]`); | |
| const hashTest = await Promise.race([ | |
| page.goto(hashURL), | |
| new Promise((_, reject) => | |
| setTimeout(() => reject(new Error(`Timeout loading page: ${hashURL}`)), timeout) | |
| ) | |
| ]); | |
| ok(hashTest.ok(), `navigated to URL [${hashURL}]`); | |
| const body = await page.locator("body").waitHandle(); | |
| let bodyHTML = await body.evaluate((el) => el.innerHTML); | |
| // If the body HTML uses "Running" to show state, wait until it changes | |
| if (bodyHTML == "<h1>Running</h1>") { | |
| while (bodyHTML === "<h1>Running</h1>") { | |
| bodyHTML = await body.evaluate((el) => el.innerHTML); | |
| } | |
| } | |
| // Attempt to parse the HTML body content as JSON | |
| const raw = bodyHTML; | |
| let testOutputJSON; | |
| try { | |
| testOutputJSON = JSON.parse(raw); | |
| } catch (err) { | |
| log(`failed to parse JSON for body HTML: ${err}`); | |
| } | |
| return { | |
| page, | |
| body, | |
| output: { | |
| raw, | |
| json: testOutputJSON, | |
| }, | |
| }; | |
| } |
| let is_async = self | ||
| .async_imports | ||
| .contains(&format!("{import_name}#{func_name}")) | ||
| || import_name | ||
| .find('@') | ||
| .map(|i| { | ||
| self.async_imports | ||
| .contains(&format!("{}#{func_name}", import_name.get(0..i).unwrap())) | ||
| }) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve async function detection logic.
The async function detection is duplicated between imports and exports. Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
+ fn is_async_function(&self, name: &str, function_name: &str) -> bool {
+ let full_name = format!("{}#{}", name, function_name);
+ self.async_imports.contains(&full_name)
+ || self.async_exports.contains(&full_name)
+ || name
+ .find('@')
+ .map(|i| {
+ let base_name = name.get(0..i).unwrap();
+ self.async_imports.contains(&format!("{}#{}", base_name, function_name))
+ || self.async_exports.contains(&format!("{}#{}", base_name, function_name))
+ })
+ .unwrap_or(false)
+ }Also applies to: 2054-2087
2 participants
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
--async-mode,--async-imports,--async-exports.Improvements
Experimental
Testing
These release notes summarize the key enhancements to async WebAssembly component handling while maintaining a high-level overview.