Conversation
WalkthroughThis pull request introduces comprehensive support for asynchronous operations in WebAssembly components, focusing on enhancing the JavaScript Component Bindgen's capabilities. The changes span multiple files and modules, adding new interfaces, types, and options for handling asynchronous imports and exports using JavaScript Promise Integration (JSPI) and Asyncify. The implementation provides flexible configuration for async modes, allowing developers to specify how components interact with asynchronous functions and resources. Changes
Sequence DiagramsequenceDiagram
participant Developer
participant JCO
participant Component
participant AsyncRuntime
Developer->>JCO: Specify async mode (JSPI/Asyncify)
JCO->>Component: Configure async imports/exports
Component->>AsyncRuntime: Prepare async function wrappers
AsyncRuntime-->>Component: Provide async execution context
Component-->>Developer: Ready for async operations
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: 35
🔭 Outside diff range comments (1)
test/fixtures/wasi/0.2.2/wasi_cli@0.2.2.wit (1)
Line range hint
275-276: Avoid Using thedeleteOperator for Performance ReasonsThe use of the
deleteoperator can negatively impact JavaScript engine optimizations and overall performance.Apply this diff to unset the
_getEntryproperty without usingdelete:const descriptorGetEntry = Descriptor.prototype._getEntry; -delete Descriptor.prototype._getEntry; +Descriptor.prototype._getEntry = undefined;This change avoids the performance cost associated with
deletewhile effectively removing the method from the prototype.
🧹 Nitpick comments (50)
crates/js-component-bindgen-component/wit/js-component-bindgen.wit (3)
74-77: Define Default Values for New Records and VariantsThe newly introduced
async-imports-exportsrecord and theasync-modevariant lack explicit default values. Consider defining default behaviors or clearly documenting the expected usage when these fields are not provided to prevent potential null reference errors or misconfigurations.
79-85: Clarify the Mode Selection LogicThe
async-modevariant now includessync,jspi, andasyncifyoptions. It would be helpful to provide additional documentation or comments explaining the scenarios in which each mode should be used, and how they impact the component generation process. This will improve maintainability and ease of use for future developers.
117-119: Synchronizeasync-modeAcross RecordsThe
async-modefield is added to bothgenerate-optionsandtype-generation-optionsrecords. Ensure that changes toasync-modein one record are appropriately reflected in the other, or consider refactoring to avoid duplication if they are intended to be the same.test/browser/webidl.js (2)
Line range hint
36-58: Handle Potential Path Resolution ErrorsThe server now serves files based on the request URL. There might be edge cases where
fileUrlis incorrectly computed, leading to file not found errors. Consider adding logging or additional error handling to assist in debugging if such cases occur.
79-83: Review Unused VariablesThe variables
browser,serverPort, andhashare passed totestBrowserPage, but if any of them are not used within the function, consider removing them or confirming their necessity. This will help clean up the code and prevent confusion.src/cmd/opt.js (1)
54-54: Document the New Optional Parameters in JSDocThe function
optimizeComponentnow acceptsasyncModeandwasmOptBinPathin theoptsparameter. Update the JSDoc comment to include descriptions for these new parameters to maintain clarity for future maintenance.packages/preview2-shim/lib/browser-async/io/streams.js (1)
23-26: Consider Explicit Null Check for Stream End in#fillBufferIn the
#fillBuffermethod, when reading from the stream, thevaluemight beundefinedif the stream has no more data. The current assignment handles this withvalue || new Uint8Array(0), but an explicit check can make the code more readable and maintainable.Here's a suggested refactor:
async #fillBuffer() { const { value, done } = await this.#reader.read(); if (done) { this.#closed = true; - this.#buffer = value || new Uint8Array(0); + this.#buffer = new Uint8Array(0); } else { + this.#buffer = value; + } }test/fixtures/wasi/0.2.2/wasi_cli@0.2.2.wit (1)
1-246: Ensure Consistency of Version AnnotationsThroughout the interfaces and world definitions, the
@since(version = 0.2.0)annotation is used. However, the package is defined aspackage wasi:cli@0.2.2;, which might cause confusion regarding the actual version of the interfaces.Consider updating the
@sinceannotations to match the package version for clarity:-@since(version = 0.2.0) +@since(version = 0.2.2) interface environment { // ... } // Apply the same change to all occurrences of @since(version = 0.2.0)packages/preview2-shim/lib/browser-async/filesystem.js (2)
113-126: ImplementwriteViaStreamor Remove Unused CodeThe method
writeViaStreamis currently unimplemented and contains commented-out code.If this functionality is planned, implement the method. Otherwise, remove the commented code to keep the codebase clean.
128-130: Provide Implementation forappendViaStreamor Remove ItThe
appendViaStreammethod contains only aconsole.logstatement.Implement the method if required. If not, consider removing it to avoid confusion.
test/browser/preview2.js (4)
30-30: Typo in comment: 'fixutres' should be 'fixtures'.There's a typo in the comment on line 30. Correct 'fixutres' to 'fixtures' for clarity.
Apply this diff to fix the typo:
-// Path to the fixutres +// Path to the fixtures
53-53: Implement mode selection for JSPI vs Asyncify.There's a TODO to allow the function to take an argument for JSPI vs Asyncify mode. Implementing this will enhance test flexibility.
Would you like assistance in adding this functionality, or should we open a new issue to track this task?
85-92: Handle errors during temporary directory cleanup.The empty
catchblock at lines 92 suppresses errors during cleanup. Consider logging the error to aid in debugging.Apply this diff to log the error:
} catch (e) { + log(`Failed to remove temp directory: ${e.message}`); }
143-143: Enable origin flag in Puppeteer for JSPI support.The TODO indicates a need to enable the origin flag in Puppeteer for JSPI. This is necessary for testing components that use JSPI.
Do you need help configuring Puppeteer to enable the origin flag for JSPI, or should we open a new issue to track this task?
test/helpers.js (1)
185-185: Simplify conditional with optional chaining.The condition
if (err && err.code && err.code === "ENOENT")can be simplified using optional chaining for better readability.Apply this diff to simplify the condition:
-if (err && err.code && err.code === "ENOENT") { +if (err?.code === "ENOENT") {🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/cli.js (2)
118-154: Consider Conditional Skipping of Async TestsThe condition
if (typeof WebAssembly.Suspending === "function")assumes that the environment supports theWebAssembly.Suspendingfeature. If this feature is not available, the tests for async modes are skipped silently. This could lead to untested code paths.Consider modifying the test suite to:
- Inform the developer when tests are skipped due to unsupported features.
- Provide guidance on how to enable or install necessary features for full test coverage.
148-152: Handle Potential Exceptions in Test AssertionsThe assertions within the async tests do not account for potential exceptions thrown by the functions under test. If
inst.runSync()orinst.runAsync()throw exceptions, the test will fail without clear diagnostic information.Wrap the function calls in try-catch blocks to capture and assert exceptions, providing clearer failure messages.
packages/preview2-shim/lib/browser-async/http/types.js (2)
251-254: Avoid Assigning to Read-Only PropertyimmutableIn the
Fieldsclass constructor, you are settingthis.immutable = immutable;, but later, ifimmutableistrue, you are throwing an error when attempting to modify headers. This could lead to confusion.Consider making
immutablea private property or using a getter method to prevent external modification.
565-571: Remove Unnecessary Constructor inRequestOptionsThe constructor in
RequestOptionsdoes not perform any operations and can be safely removed to simplify the code.Apply this diff to remove the unnecessary constructor:
export class RequestOptions { - constructor() {} connectTimeout() { return; }🧰 Tools
🪛 Biome (1.9.4)
[error] 565-565: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
test/fixtures/wasi/0.2.2/wasi_filesystem@0.2.2.wit (1)
534-534: Ensure Consistency in Function NamingThe function
metadata-hash-atuses a hyphen, whereas other functions use camelCase or snake_case naming conventions.Consider renaming the function for consistency:
- metadata-hash-at: func(path-flags: path-flags, path: string) -> result<metadata-hash-value, error-code>; + metadataHashAt: func(path-flags: path-flags, path: string) -> result<metadata-hash-value, error-code>;test/fixtures/wasi/0.2.2/wasi_http@0.2.2.wit (1)
551-551: Typographical error in documentation commentThere is a typo in the comment at line 551. The word "inducating" should be corrected to "indicating".
Apply this diff to fix the typo:
- /// empty) set of trailers, inducating the full contents of the body + /// empty) set of trailers, indicating the full contents of the bodycrates/js-component-bindgen/src/intrinsics.rs (1)
122-184: Refactor duplicated code inAsyncifyAsyncInstantiateandAsyncifySyncInstantiateThe implementations of
Intrinsic::AsyncifyAsyncInstantiate(lines 122-152) andIntrinsic::AsyncifySyncInstantiate(lines 154-184) contain significant duplicated code. Refactoring the common logic into a shared function would improve maintainability and reduce redundancy.Consider extracting the shared code into a helper function or combining the variants if possible.
packages/preview2-shim/lib/browser-async/io.js (1)
3-4: Consider naming convention for error exportsIn the exported
errorobject, the property is namedError. It's common practice in JavaScript to use lowercase for object property names unless they are constructors. Consider renaming it toerrorfor consistency.Apply this diff for consistency:
-export const error = { Error: IoError }; +export const error = { error: IoError };If
IoErroris intended to be used as a constructor, andErroris deliberately capitalized, you may disregard this suggestion.test/browser/test.js (1)
6-11: Add error handling for platform check.The platform check could benefit from additional error handling to ensure graceful behavior if
platformis undefined.export async function browserTest() { - if (platform !== 'win32') { + if (typeof platform === 'undefined') { + console.warn('Platform information unavailable, skipping browser tests'); + return; + } + if (platform !== 'win32') { // await browserWebIdlTest(); await browserPreview2Test(); } }packages/preview2-shim/lib/browser-async/http/outgoing-handler.js (1)
5-13: Complete the JSDoc documentation.The JSDoc comments are incomplete:
- Missing documentation for the
_optionsparameter- Return type is documented as
voidbut the function returns aFutureIncomingResponseUpdate the JSDoc as follows:
/** * Polyfill for handling an outgoing wasi:http request (i.e. `wasi:http/outgoing-handler.handle`) * * Generally this is resolved by making an external request (`fetch`, XMLHTTPRequest) * * @param {OutgoingRequest} incomingRequest * @param {ResponseOutparam} responseOutparam + * @param {Object} [_options] - Optional configuration for the handler - * @returns void + * @returns {FutureIncomingResponse} */packages/preview2-shim/lib/browser-async/random/random.js (1)
4-4: Use a more descriptive constant name.
MAX_BYTEScould be more descriptive to indicate it's a crypto API limitation.-const MAX_BYTES = 65536; +const CRYPTO_API_MAX_BYTES = 65536;packages/preview2-shim/lib/browser-async/clocks/wall-clock.js (3)
3-7: Fix typo in JSDoc typedef.There's a typo in the @typedef tag.
/** * A time and date in seconds plus nanoseconds. * - * @typdef{{seconds: bigint, nanoseconds: number}} Datetime + * @typedef {{seconds: bigint, nanoseconds: number}} Datetime */
23-28: Improve time precision using performance.now().
Date.now()only provides millisecond precision. Usingperformance.now()would provide microsecond precision.export const now = () => { - const now = Date.now(); // in milliseconds + const now = performance.now(); // in milliseconds with microsecond precision const seconds = BigInt(Math.floor(now / 1e3)); - const nanoseconds = (now % 1e3) * 1e6; + const nanoseconds = Math.floor((now % 1e3) * 1e6); return { seconds, nanoseconds }; };
37-39: Update resolution to match performance.now() precision.The resolution should reflect the actual precision of the time source.
export const resolution = () => { - return { seconds: 0n, nanoseconds: 1e6 }; + return { seconds: 0n, nanoseconds: 1e3 }; // performance.now() provides microsecond precision };packages/preview2-shim/lib/browser-async/http/incoming-handler.js (2)
19-27: Complete the JSDoc documentation forgenHandler.The JSDoc comment is incomplete:
- Missing description of the return value
- Incorrect return type (
@returns void)- Missing
@param {Request} reqfor the returned function's parameterUpdate the JSDoc as follows:
/** * Helper function that given a wasi:http compliant incoming-handler function, * calls the function with native Web platform `Request`s, waits for the response to be computed, - * and returns the + * and returns the response converted to the appropriate format. * * @see https://developer.mozilla.org/en-US/docs/Web/API/Request/Request * @param {Function} handle - A function that adheres to the WASI HTTP spec for incoming-handler - * @returns void + * @returns {Function} An async function that takes a Request and returns a Response + * @param {Request} req - The incoming Web platform Request object */
28-36: Improve error handling ingenHandler.The function throws the raw result object which may expose internal implementation details.
Consider wrapping the error in a more user-friendly format:
if (result.tag !== "ok") { - throw result; // error + throw new Error(`Failed to handle request: ${result.val || 'Unknown error'}`); }test/test.js (2)
1-14: Update file header comments to include async test information.The file header comments should be updated to include information about the async test functionality.
Add a description of the async test to the header comments:
* When the runtime test is present, the flags in the runtime host.ts file will be used * as the flags of the code generation step. + * + * The async test verifies the correct behavior of asynchronous components and their + * integration with the runtime. */
Line range hint
22-31: Group import statements by type.Consider organizing imports by separating external (node) and internal (local) imports.
Reorganize imports as follows:
+ // External imports import { browserTest } from './browser/test.js'; + + // Internal test imports import { codegenTest } from './codegen.js'; import { runtimeTest } from './runtime.js';src/cmd/componentize.js (1)
5-17: Add @throws to JSDoc documentation.The JSDoc should document potential errors that may be thrown.
Add @throws documentation:
* @param {string} [opts.preview2Adapter] - Path to a custom preview2 adapter * @param {string} opts.out - Path to which to write the WebAssembly component output + * @throws {Error} When source file cannot be read or component creation fails */packages/preview2-shim/lib/browser-async/clocks/monotonic-clock.js (1)
26-29: Optimize time conversion and prevent precision loss.The current implementation may lose precision during conversion.
Consider using BigInt multiplication for better precision:
- return BigInt(Math.floor(performance.now() * 1e6)); + return BigInt(Math.round(performance.now())) * BigInt(1e6);packages/preview2-shim/lib/browser-async/io/poll.js (2)
57-62: Consider adding error handling for empty input list.The optimization for single pollable looks good, but the function should handle empty input lists.
export const poll = async (inList) => { + if (!inList?.length) { + return new Uint32Array(0); + } if (inList.length === 1) { // handle this common case faster await inList[0].block();
64-78: Consider memory optimization for large input lists.The current implementation allocates space for all pollables initially. For large lists, consider pre-filtering ready pollables or using a dynamic array.
packages/preview2-shim/lib/browser-async/sockets.js (1)
40-110: Consider implementing TCP operations incrementally.The TCP interface has many methods. Consider implementing and testing them incrementally, starting with essential operations like
connect,bind, andlisten.src/common.js (1)
Line range hint
96-127: Enhance error handling and cleanup.While the cleanup is more robust with the
tmpDircheck, consider these improvements:
- Add error type information to the rejected promise
- Handle potential cleanup errors
Apply this diff to enhance error handling:
await p; var output = await readFile(outFile); return output; } finally { if (tmpDir) { - await rm(tmpDir, { recursive: true }); + try { + await rm(tmpDir, { recursive: true }); + } catch (cleanupError) { + console.warn('Failed to clean up temporary directory:', cleanupError); + } } } }test/fixtures/wasi/0.2.2/wasi_clocks@0.2.2.wit (1)
64-93: Consider simplifying resolution return type.The resolution function returns a datetime record, but since resolution is a duration concept, it might be simpler and more efficient to return a u64 nanoseconds value like the monotonic clock's resolution function.
test/async.js (1)
144-146: Address TODO comments.These TODO comments indicate incomplete implementations for:
- RequestOption implementation
- Pollable reuse functionality
- Browser-async sockets error handling
Would you like me to help implement any of these features?
src/jco.js (1)
85-89: Consider extracting shared async options configuration.The async options are duplicated between
transpileandtypescommands. Consider extracting these into a shared configuration to maintain consistency and reduce duplication.+ const asyncOptions = [ + new Option('--async-mode [mode]', 'EXPERIMENTAL: use async imports and exports') + .choices(['sync', 'jspi', 'asyncify']) + .preset('sync'), + new Option('--default-async-imports', 'EXPERIMENTAL: default async component imports from WASI interfaces'), + new Option('--default-async-exports', 'EXPERIMENTAL: default async component exports from WASI interfaces'), + new Option('--async-imports <imports...>', 'EXPERIMENTAL: async component imports (examples: "wasi:io/poll@0.2.0#poll", "wasi:io/poll#[method]pollable.block")'), + new Option('--async-exports <exports...>', 'EXPERIMENTAL: async component exports (examples: "wasi:cli/run@#run", "handle")') + ]; program.command('transpile') // ... other options ... - .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")') + .addOptions(asyncOptions) program.command('types') // ... other options ... - .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")') + .addOptions(asyncOptions)src/cmd/transpile.js (1)
79-82: Add null checks for more robust option handling.While the code handles undefined values, it could be more robust with explicit null checks.
- if (opts.defaultAsyncImports) - opts.asyncImports = DEFAULT_ASYNC_IMPORTS.concat(opts.asyncImports || []); - if (opts.defaultAsyncExports) - opts.asyncExports = DEFAULT_ASYNC_EXPORTS.concat(opts.asyncExports || []); + if (opts.defaultAsyncImports) { + opts.asyncImports = DEFAULT_ASYNC_IMPORTS.concat(opts.asyncImports ?? []); + } + if (opts.defaultAsyncExports) { + opts.asyncExports = DEFAULT_ASYNC_EXPORTS.concat(opts.asyncExports ?? []); + }crates/js-component-bindgen/src/function_bindgen.rs (1)
1085-1089: Consider extracting common call formatting logic.The async call formatting is duplicated between dynamic and regular calls. Consider extracting this into a helper method.
+ fn format_async_call(&self, callee: &str, args: &str) -> String { + let maybe_async_await = if self.is_async { "await " } else { "" }; + format!("{}{callee}({args})", maybe_async_await) + } // In CallInterface: - format!( - "{maybe_async_await}{}({})", - self.callee, - operands.join(", ") - ) + self.format_async_call(&self.callee, &operands.join(", "))package.json (1)
Line range hint
62-79: Consider using fixed versions for critical dependencies.Using caret (^) for version ranges could lead to unexpected breaking changes. Consider using fixed versions for critical dependencies, especially for packages that might affect the async functionality.
test/browser/browser-preview2.html (1)
47-49: Consider enhancing error handling.The error handling could be improved by:
- Adding more specific error types/messages for different failure scenarios
- Including error recovery mechanisms
- Adding detailed logging for debugging purposes
Also applies to: 51-53
docs/src/contributing.md (1)
64-67: Fix formatting and grammar issues.
- Add period after "etc" in line 64
- Remove duplicate "is" in line 67
- Consider adding more detailed descriptions for environment variables
🧰 Tools
🪛 LanguageTool
[style] ~64-~64: In American English, abbreviations like “etc.” require a period.
Context: ...ol whether debugging information (logs, etc) is turned on during test ...(ETC_PERIOD)
[duplication] ~67-~67: Possible typo: you repeated a word.
Context: ...lls the tests whether the custom engine is is preoptimized ...(ENGLISH_WORD_REPEAT_RULE)
test/fixtures/browser/test-pages/wasi-http-incoming-handler.guest-export.async.preview2.html (1)
106-108: Add resource cleanup.The test creates a Request object and performs I/O operations but doesn't properly clean up resources. Consider adding cleanup code in a try-finally block.
docs/src/transpiling.md (2)
56-58: Fix formatting inconsistencies.The new options should maintain consistent formatting with the rest of the documentation. Add a space after each bullet point.
-* `--async-mode [mode]`: 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`). -* `--async-imports <imports...>`: Specify the component imports as `async`. Used with `--async-mode`. -* `--async-exports <exports...>`: Specify the component exports as `async`. Used with `--async-mode`. +* `--async-mode [mode]`: 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`). +* `--async-imports <imports...>`: Specify the component imports as `async`. Used with `--async-mode`. +* `--async-exports <exports...>`: Specify the component exports as `async`. Used with `--async-mode`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~56-~56: Loose punctuation mark.
Context: ...with typescript *--async-mode [mode]: For the component imports and exports, ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~57-~57: Loose punctuation mark.
Context: ...cify). *--async-imports <imports...>: Specify the component imports asasync...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~58-~58: Loose punctuation mark.
Context: ...-mode. *--async-exports <exports...>: Specify the component exports asasync...(UNLIKELY_OPENING_PUNCTUATION)
56-59: Add usage examples for async options.Consider adding examples demonstrating how to use these options together, similar to other sections in the documentation. This would help users understand how to properly configure async functionality.
Here's a suggested example to add after the options:
### Async Usage Example To use async functionality with JSPI: ```bash jco transpile component.wasm \ --async-mode jspi \ --async-imports "my:pkg/interface.handler" \ --async-exports "my:pkg/interface.process"This will mark the
handlerimport andprocessexport as async functions using JavaScript Promise Integration.<details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [uncategorized] ~56-~56: Loose punctuation mark. Context: ...with typescript * `--async-mode [mode]`: For the component imports and exports, ... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~57-~57: Loose punctuation mark. Context: ...cify`). * `--async-imports <imports...>`: Specify the component imports as `async... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~58-~58: Loose punctuation mark. Context: ...-mode`. * `--async-exports <exports...>`: Specify the component exports as `async... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between eefa6f321695b7d3fda0476e6f6d4119ed055643 and 70fcd8daf8243adc79d5aad9fa1008166b74a78a. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `test/fixtures/components/async_call.component.wasm` is excluded by `!**/*.wasm` </details> <details> <summary>📒 Files selected for processing (62)</summary> * `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` (15 hunks) * `crates/wasm-tools-component/src/lib.rs` (1 hunks) * `docs/src/contributing.md` (1 hunks) * `docs/src/transpiling.md` (1 hunks) * `package.json` (3 hunks) * `packages/preview2-shim/lib/browser-async/cli.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/clocks.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/clocks/monotonic-clock.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/clocks/timezone.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/clocks/wall-clock.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/filesystem.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/http.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/http/incoming-handler.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/http/outgoing-handler.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/http/types.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/index.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/io.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/io/error.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/io/poll.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/io/streams.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/random.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/random/insecure.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/random/random.js` (1 hunks) * `packages/preview2-shim/lib/browser-async/sockets.js` (1 hunks) * `packages/preview2-shim/lib/browser/cli.js` (1 hunks) * `packages/preview2-shim/lib/browser/random.js` (1 hunks) * `packages/preview2-shim/package.json` (1 hunks) * `src/api.js` (1 hunks) * `src/cmd/componentize.js` (1 hunks) * `src/cmd/opt.js` (5 hunks) * `src/cmd/transpile.js` (8 hunks) * `src/common.js` (2 hunks) * `src/jco.js` (3 hunks) * `test/async.js` (1 hunks) * `test/browser.html` (0 hunks) * `test/browser/browser-preview2.html` (1 hunks) * `test/browser/preview2.js` (1 hunks) * `test/browser/test.js` (1 hunks) * `test/browser/webidl.js` (6 hunks) * `test/cli.js` (33 hunks) * `test/codegen.js` (1 hunks) * `test/fixtures/browser/test-pages/wasi-http-incoming-handler.guest-export.async.preview2.html` (1 hunks) * `test/fixtures/components/js/browser-incoming-handler/component.js` (1 hunks) * `test/fixtures/components/js/browser-incoming-handler/component.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_cli@0.2.2.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_clocks@0.2.2.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_filesystem@0.2.2.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_http@0.2.2.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_io@0.2.2.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_random@0.2.2.wit` (1 hunks) * `test/fixtures/wasi/0.2.2/wasi_sockets@0.2.2.wit` (1 hunks) * `test/helpers.js` (3 hunks) * `test/test.js` (4 hunks) * `xtask/src/build/jco.rs` (1 hunks) * `xtask/src/generate/wasi_types.rs` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * test/browser.html * crates/js-component-bindgen/src/core.rs </details> <details> <summary>✅ Files skipped from review due to trivial changes (8)</summary> * test/fixtures/components/js/browser-incoming-handler/component.wit * packages/preview2-shim/lib/browser-async/random.js * packages/preview2-shim/lib/browser/cli.js * packages/preview2-shim/lib/browser/random.js * packages/preview2-shim/lib/browser-async/http.js * packages/preview2-shim/lib/browser-async/clocks.js * src/api.js * packages/preview2-shim/lib/browser-async/index.js </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/src/contributing.md</summary> [uncategorized] ~53-~53: Loose punctuation mark. Context: ...test --workspace packages/preview2-shim`: `preview2-shim` unit tests. * `test/bro... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~54-~54: Loose punctuation mark. Context: ...-shim` unit tests. * `test/browser.html`: Bare-minimum browser validation test. *... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~55-~55: Loose punctuation mark. Context: ... browser validation test. * `cargo test`: Wasmtime preview2 conformance tests (no... (UNLIKELY_OPENING_PUNCTUATION) --- [style] ~64-~64: In American English, abbreviations like “etc.” require a period. Context: ...ol whether debugging information (logs, etc) is turned on during test ... (ETC_PERIOD) --- [duplication] ~67-~67: Possible typo: you repeated a word. Context: ...lls the tests whether the custom engine is is preoptimized ... (ENGLISH_WORD_REPEAT_RULE) --- [style] ~72-~72: In American English, abbreviations like “etc.” require a period. Context: ...t of tests, headless puppeteer browser, etc). To do that you can use `TEST_DEBUG`: ... (ETC_PERIOD) --- [formatting] ~72-~72: Consider inserting a comma after ‘that’. Context: ...ests, headless puppeteer browser, etc). To do that you can use `TEST_DEBUG`: ```console T... (TO_VERB_COMMA) </details> <details> <summary>docs/src/transpiling.md</summary> [uncategorized] ~56-~56: Loose punctuation mark. Context: ...with typescript * `--async-mode [mode]`: For the component imports and exports, ... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~57-~57: Loose punctuation mark. Context: ...cify`). * `--async-imports <imports...>`: Specify the component imports as `async... (UNLIKELY_OPENING_PUNCTUATION) --- [uncategorized] ~58-~58: Loose punctuation mark. Context: ...-mode`. * `--async-exports <exports...>`: Specify the component exports as `async... (UNLIKELY_OPENING_PUNCTUATION) </details> </details> <details> <summary>🪛 Biome (1.9.4)</summary> <details> <summary>src/cmd/componentize.js</summary> [error] 19-19: eval() exposes to security risks and performance issues. See the MDN web docs for more details. Refactor the code so that it doesn't need to call eval(). (lint/security/noGlobalEval) </details> <details> <summary>packages/preview2-shim/lib/browser-async/filesystem.js</summary> [error] 275-276: Avoid the delete operator which can impact performance. Unsafe fix: Use an undefined assignment instead. (lint/performance/noDelete) </details> <details> <summary>src/cmd/opt.js</summary> [error] 179-179: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) </details> <details> <summary>packages/preview2-shim/lib/browser-async/http/types.js</summary> [error] 565-565: This constructor is unnecessary. Unsafe fix: Remove the unnecessary constructor. (lint/complexity/noUselessConstructor) --- [error] 723-725: This constructor is unnecessary. Unsafe fix: Remove the unnecessary constructor. (lint/complexity/noUselessConstructor) </details> <details> <summary>packages/preview2-shim/lib/browser-async/cli.js</summary> [error] 15-15: The assignment should not be in an expression. The use of assignments in expressions is confusing. Expressions are often considered as side-effect free. (lint/suspicious/noAssignInExpressions) </details> <details> <summary>test/helpers.js</summary> [error] 185-185: Change to an optional chain. Unsafe fix: Change to an optional chain. (lint/complexity/useOptionalChain) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (11)</summary> * GitHub Check: Jco Node.js WASI Conformance Tests (macos-latest, latest) * GitHub Check: Jco Node.js WASI Conformance Tests (windows-latest, latest) * GitHub Check: Jco Test Suite (macos-latest, latest) * GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, latest) * GitHub Check: Jco Test Suite (windows-latest, 20.x) * GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, 20.x) * GitHub Check: Jco Test Suite (ubuntu-22.04, latest) * GitHub Check: Jco Test Suite (ubuntu-22.04, 20.x) * GitHub Check: Jco Node.js WASI Conformance Tests (ubuntu-latest, 18.x) * GitHub Check: Jco Test Suite (ubuntu-22.04, 18.x) * GitHub Check: Jco Deno WASI Conformance Tests </details> <details> <summary>🔇 Additional comments (44)</summary><blockquote> <details> <summary>crates/js-component-bindgen-component/wit/js-component-bindgen.wit (1)</summary> `69-71`: **Ensure Consistency with Existing Configuration Options** The addition of the `async-mode` field in `generate-options` is a significant change. Please verify that this new option integrates seamlessly with existing options and does not conflict with other configuration settings like `instantiation` and `import-bindings`. </details> <details> <summary>test/browser/webidl.js (1)</summary> `118-122`: **Ensure Correct Hash Values in Test Cases** Verify that the `hash` values provided (`'test:dom.js'` and `'test:console.js'`) correspond to the correct test cases and that they are properly handled within `testBrowserPage`. Mismatched hash values could lead to tests not executing as intended. </details> <details> <summary>src/cmd/opt.js (1)</summary> `78-79`: **Conditional Addition of Asyncify Argument** The code conditionally adds the `--asyncify` argument when `asyncMode` is `'asyncify'`. Ensure that this logic correctly reflects the desired behavior and that no other async modes require additional arguments. This helps prevent unintended side effects during optimization. </details> <details> <summary>test/fixtures/wasi/0.2.2/wasi_cli@0.2.2.wit (1)</summary> `44-46`: **Verify Import Paths for Dependencies** In the interfaces `stdin`, `stdout`, and `stderr`, the `use` statements reference `wasi:io/streams@0.2.2.{input-stream}` and similar paths. Ensure that the referenced versions and paths are correct and that the dependencies are available in the specified versions. Run the following script to confirm: Also applies to: 52-55, 61-64 <details> <summary>✅ Verification successful</summary> <!-- [monologue] [task verify_imports_1/1] [observations] 1. Found comprehensive evidence of streams-related imports and interfaces: - The streams interface is defined in wasi_io@0.2.2.wit - Multiple files correctly import streams from wasi:io/streams@0.2.2 - Version 0.2.2 is consistently used across all imports 2. Specific to wasi_cli@0.2.2.wit: - Correctly uses `wasi:io/streams@0.2.2.{input-stream}` for stdin - Correctly uses `wasi:io/streams@0.2.2.{output-stream}` for stdout/stderr - All imports have proper version annotations (@since version = 0.2.0) 3. The streams interface is properly defined and imported by other WASI modules [analysis] 1. All import paths are correct and consistent 2. The version numbers (0.2.2) match across all files 3. The dependencies are available in the specified versions 4. The import structure follows the expected pattern across the codebase [actions] 1. Generate verification success response as all imports are correct and consistent --> **Import paths and dependencies verified successfully** ✅ All `wasi:io/streams@0.2.2` imports are correct and consistent across the codebase. The streams interface is properly defined in `wasi_io@0.2.2.wit` and correctly imported by the CLI and other WASI modules. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify that the imported interfaces exist in version 0.2.2 # Check if the interfaces are available grep -rq "interface streams" test/fixtures/wasi/0.2.2Length of output: 55
Script:
#!/bin/bash # Search for all WIT files and their content in the 0.2.2 directory echo "=== Searching for WIT files ===" fd -e wit . test/fixtures/wasi/0.2.2 echo -e "\n=== Checking streams-related imports and interfaces ===" rg -l "streams" test/fixtures/wasi/0.2.2 echo -e "\n=== Checking actual import statements ===" rg "(use|import).*streams.*0\.2\.2" test/fixtures/wasi/0.2.2 -A 2 echo -e "\n=== Checking interface definitions ===" rg "interface (streams|input-stream|output-stream)" test/fixtures/wasi/0.2.2 -A 2Length of output: 4186
test/fixtures/wasi/0.2.2/wasi_io@0.2.2.wit (1)
1-331: Interface definitions look good.
The WIT package wasi:io@0.2.2 interface definitions and resource declarations are well-structured and documented. The code accurately defines the error, poll, and streams interfaces.
test/cli.js (1)
13-13: Ensure Compatibility of AsyncFunction Definition
The declaration of AsyncFunction using (async () => {}).constructor may not be compatible with all environments, particularly older Node.js versions. Consider adding a check or providing a fallback to ensure compatibility across different environments.
packages/preview2-shim/lib/browser-async/http/types.js (1)
723-725: Clarify the Necessity of the Constructor in NotReadyYetError
The constructor in NotReadyYetError simply calls the parent Error constructor with the message. While the linter suggests it is unnecessary, keeping it clarifies the intention and ensures proper error handling.
This issue has been addressed previously, and the current implementation is appropriate.
🧰 Tools
🪛 Biome (1.9.4)
[error] 723-725: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
test/fixtures/wasi/0.2.2/wasi_filesystem@0.2.2.wit (1)
451-451:
Correct the Function Signature of link-at
The link-at function is missing the required path-flags parameter for the old-path. This could lead to inconsistency and potential errors when following symlinks.
Apply this diff to include the path-flags parameter:
link-at: func(
+ old-path-flags: path-flags,
old-path: string,
new-descriptor: borrow<descriptor>,
new-path: string
) -> result<_, error-code>;Likely invalid or redundant comment.
crates/js-component-bindgen/src/ts_bindgen.rs (1)
Line range hint 809-915: Ensure proper wrapping of return types with Promise in asynchronous functions
The code correctly wraps return types with Promise<> when is_async is true. However, verify that all asynchronous functions consistently apply the Promise wrapper to their return types to prevent type mismatches in the generated TypeScript bindings.
Run the following script to check for functions missing the Promise wrapper:
✅ Verification successful
Proper Promise wrapping verified in async function type generation
The code correctly and consistently wraps return types with Promise<> for all async functions. The implementation ensures that whenever is_async is true:
- The function is marked with the
asynckeyword - The return type is wrapped in
Promise<> - The Promise wrapper is properly closed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for async functions that do not wrap return types with Promise.
# Search for function definitions where is_async is true but 'Promise<' is not added to return type.
ast-grep --pattern $'
fn ts_func($_, $_, $_, is_async) {
$$$
if is_async {
$$$
}
$$$
}
' crates/js-component-bindgen/src/ts_bindgen.rs
Length of output: 147
Script:
#!/bin/bash
# Search for all occurrences of is_async and Promise handling
rg -A 5 -B 5 'is_async.*Promise' crates/js-component-bindgen/src/ts_bindgen.rs
# Also search for any other async-related type generation
rg -A 5 'if\s+is_async' crates/js-component-bindgen/src/ts_bindgen.rs
Length of output: 744
test/fixtures/wasi/0.2.2/wasi_sockets@0.2.2.wit (1)
712-712:
Typo in function name %stream
The function name %stream contains an unexpected % character. It should likely be stream.
Apply this diff to fix the typo:
- %stream: func(remote-address: option<ip-socket-address>) -> result<tuple<incoming-datagram-stream, outgoing-datagram-stream>, error-code>;
+ stream: func(remote-address: option<ip-socket-address>) -> result<tuple<incoming-datagram-stream, outgoing-datagram-stream>, error-code>;Likely invalid or redundant comment.
crates/js-component-bindgen/src/transpile_bindgen.rs (3)
73-76: Enhancement: Added async_mode to TranspileOpts
The addition of the async_mode field in the TranspileOpts struct enables configuration for asynchronous operations, enhancing flexibility in handling async imports and exports.
78-89: Definition of AsyncMode enum improves code clarity
Introducing the AsyncMode enum with variants Sync, JavaScriptPromiseIntegration, and Asyncify provides a clear structure for managing different asynchronous modes.
2069-2072: Consistent use of async keyword in function declarations
The variable maybe_async correctly prefixes functions with async when is_async is true, ensuring asynchronous functions are properly declared.
packages/preview2-shim/lib/browser-async/io.js (1)
6-7: Exporting modules using export * as syntax
The use of export * as poll and export * as streams is appropriate and aligns with the modern ES module syntax, promoting modularity and reusability.
test/browser/test.js (1)
3-4: Consider documenting why WebIDL test is commented out.
The commented-out browserWebIdlTest import and its usage suggest incomplete or pending test implementation. If this is intentional, please add a comment explaining why it's disabled.
Also applies to: 8-8
packages/preview2-shim/lib/browser-async/random/insecure.js (1)
4-4: Verify the imported functions exist.
Let's verify that the imported functions are properly implemented in the random.js file.
✅ Verification successful
✓ Imported functions verified and properly implemented
Both getRandomBytes and getRandomU64 are correctly exported from random.js and use secure implementations based on crypto.getRandomValues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the imported functions exist in random.js
ast-grep --pattern 'export const getRandomBytes = $_' ./packages/preview2-shim/lib/browser-async/random/random.js
ast-grep --pattern 'export const getRandomU64 = $_' ./packages/preview2-shim/lib/browser-async/random/random.js
Length of output: 2144
packages/preview2-shim/lib/browser-async/http/incoming-handler.js (1)
15-17: LGTM: Clear error message for unimplemented feature.
The error message clearly explains why the feature is unimplemented in browser environments.
packages/preview2-shim/lib/browser-async/clocks/monotonic-clock.js (1)
37-40: LGTM: Clear resolution specification.
The resolution is clearly defined as 1 millisecond (1e6 nanoseconds).
xtask/src/generate/wasi_types.rs (1)
42-42: Verify async mode integration with transpiler.
The addition of async_mode looks good, maintaining backward compatibility with None as default.
Let's verify the async mode integration:
✅ Verification successful
Async mode integration verified successfully
The async_mode field is properly integrated across the codebase:
- Consistent handling in transpiler and TypeScript bindings
- Proper defaults and backward compatibility
- Complete build system integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent async mode handling across the codebase
# Check for async mode usage in transpiler code
rg -l "async_mode" --type rust
Length of output: 234
Script:
#!/bin/bash
# Check async mode implementation details
# Check transpile_bindgen.rs for async mode handling
echo "=== transpile_bindgen.rs ==="
rg "async_mode" crates/js-component-bindgen/src/transpile_bindgen.rs -B 2 -A 2
# Check ts_bindgen.rs for async mode usage
echo -e "\n=== ts_bindgen.rs ==="
rg "async_mode" crates/js-component-bindgen/src/ts_bindgen.rs -B 2 -A 2
# Check component lib.rs
echo -e "\n=== component/lib.rs ==="
rg "async_mode" crates/js-component-bindgen-component/src/lib.rs -B 2 -A 2
# Check build system integration
echo -e "\n=== build/jco.rs ==="
rg "async_mode" xtask/src/build/jco.rs -B 2 -A 2
Length of output: 1743
packages/preview2-shim/lib/browser-async/io/poll.js (1)
4-42: LGTM! Well-structured Pollable class implementation.
The class effectively encapsulates polling state with proper documentation and modern JavaScript features.
test/codegen.js (2)
29-29: LGTM! Improved test file handling logic.
The new naming and filtering approach provides better flexibility for handling multiple test files per fixture.
Also applies to: 31-32
34-38: Consider adding test name validation.
The test name is used in file paths. Consider validating it to prevent path traversal or invalid characters.
xtask/src/build/jco.rs (1)
88-88: LGTM! The async mode configuration is well-integrated.
The addition of the optional async_mode field to TranspileOpts provides a clean way to configure async behavior while maintaining backward compatibility.
test/fixtures/wasi/0.2.2/wasi_random@0.2.2.wit (1)
1-92: LGTM! Well-designed random interfaces with clear security boundaries.
The implementation demonstrates excellent practices:
- Clear separation between secure and insecure operations
- Comprehensive documentation with security warnings
- Proper versioning with
@sinceannotations - Future-proofing with evolution notes
src/common.js (1)
80-95: LGTM! Clear and comprehensive documentation.
The JSDoc documentation clearly explains the command format, parameters, and usage requirements.
test/fixtures/wasi/0.2.2/wasi_clocks@0.2.2.wit (2)
1-47: Well-designed monotonic clock interface with proper async support!
The interface is thoroughly documented, uses appropriate types for nanosecond precision, and correctly implements async functionality through pollable returns.
95-103: LGTM! Imports are well-structured.
The imports world correctly includes all necessary dependencies with proper versioning.
test/async.js (1)
72-106: LGTM! Well-structured JSPI test case.
The test properly verifies both synchronous and asynchronous function behavior with appropriate assertions.
crates/js-component-bindgen-component/src/lib.rs (2)
56-68: LGTM! Clean AsyncMode conversion implementation.
The From trait implementation correctly handles all AsyncMode variants with proper mapping to js_component_bindgen::AsyncMode.
93-93: LGTM! Consistent async_mode field addition.
The async_mode field is consistently added to TranspileOpts in both generate and generate_types methods.
Also applies to: 180-180
crates/js-component-bindgen/src/lib.rs (1)
11-11: LGTM! Appropriate public export of AsyncMode.
The AsyncMode type is correctly exposed alongside related transpilation types.
crates/wasm-tools-component/src/lib.rs (1)
187-192: LGTM! Enhanced error message with binary size context.
The improved error message now includes the binary size, which helps in debugging issues related to binary data handling.
src/jco.js (2)
37-37: LGTM! Well-structured engine customization option.
The --engine option follows the CLI's pattern and provides clear documentation about its purpose.
56-60: LGTM! Comprehensive async support options with clear documentation.
The new async options are well-structured and include helpful examples. Consider adding validation for the format of async imports/exports to ensure they match the expected pattern (e.g., "wasi:io/poll@0.2.0#poll").
Run this script to check if the async imports/exports format is validated:
✅ Verification successful
Format validation exists for async-mode but is acceptable for imports/exports
The async-mode option is properly validated through Commander.js's choices constraint. While there's no explicit format validation for async imports/exports, this is acceptable for experimental features as:
- The expected format is well-documented with clear examples
- Invalid formats would fail gracefully during processing
- Test coverage demonstrates correct usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's validation for async imports/exports format
# Search for validation logic
rg -A 5 "async-?(imports|exports)" | grep -i "validate\|check\|match\|pattern\|format"
Length of output: 88
Script:
#!/bin/bash
# Find where async options are processed and any related validation
# Look for async-related option processing
rg -l "async-mode|async-imports|async-exports" --type js
# Then find the actual implementation
ast-grep --pattern 'function $_($$_) {
$$$
async-$$$
$$$
}'
Length of output: 174
Script:
#!/bin/bash
# Examine implementation and tests for async options
# Check implementation in src/jco.js
rg -A 10 "async-mode|async-imports|async-exports" src/jco.js
# Check test cases
rg -A 10 "async-mode|async-imports|async-exports" test/cli.js test/browser/preview2.js
Length of output: 4690
src/cmd/transpile.js (4)
17-31: LGTM! Well-organized default async configurations.
The default async imports and exports are clearly organized by WASI interface and use consistent naming patterns.
50-52: LGTM! Well-documented function parameters.
The JSDoc comments are properly updated with the new async options, maintaining consistent documentation style.
84-92: LGTM! Clear and consistent async mode configuration.
The async mode configuration is well-structured with clear conditions and consistent property naming.
205-210: LGTM! Well-documented optimization condition.
The optimization condition is clearly explained with detailed comments and properly handles the asyncify case.
crates/js-component-bindgen/src/function_bindgen.rs (3)
89-89: LGTM! Clean addition of async support field.
The is_async field is well-named and follows the struct's field ordering pattern.
1052-1058: LGTM! Clean implementation of conditional await.
The conditional await prefix is cleanly implemented with clear variable naming and consistent formatting.
1076-1079: LGTM! Consistent async handling for dynamic resources.
The async handling for dynamic resources follows the same clean pattern used in other call types.
packages/preview2-shim/package.json (1)
19-25: LGTM! Well-structured async exports.
The new async exports are properly configured with corresponding type definitions and implementations, maintaining consistency with the existing export patterns.
package.json (1)
54-54: Verify if the increased timeout is necessary.
The test timeout has been significantly increased from 2 minutes to 10 minutes. While this might be necessary for async operations, consider profiling the tests to identify if there are any performance bottlenecks that could be optimized.
test/browser/browser-preview2.html (1)
41-42:
Fix template literal in error message.
There's a bug in the error message template literal. It's using ${err} instead of ${e}.
- err(e.toString() + `\nrunning "${testModule}"`);
+ err(`${e}\nrunning "${testModule}"`);Likely invalid or redundant comment.
| import { transpile } from '../../src/api.js'; | ||
| import { exec, jcoPath, testBrowserPage, getTmpDir, getRandomPort } from "../helpers.js"; | ||
|
|
||
| export async function browserWebIdlTest() { |
There was a problem hiding this comment.
Update Exported Function Name in Related Modules
The function browserTest has been renamed to browserWebIdlTest. Please ensure that all imports and usages of this function in other test suites or scripts are updated accordingly to prevent reference errors.
| return await spawnIOTmp(wasmOptPath, moduleBytes, [...cliArgs, '-o']); | ||
| } catch (e) { | ||
| if (e.toString().includes('BasicBlock requested')) | ||
| return wasmOpt(source, args); | ||
| if (e.toString().includes('BasicBlock requested')) { | ||
| return wasmOpt(args); | ||
| } |
There was a problem hiding this comment.
Avoid Recursive Calls Without Termination
The recursive call to wasmOpt(args) within the catch block at line 190 could lead to infinite recursion if the error condition persists. Ensure there's a base case or termination condition to prevent stack overflow errors.
Consider handling the error differently or limiting the number of retries.
| try { | ||
| await stat(wasmOptPath); | ||
| } catch (err) { | ||
| if (err && err.code && err.code === 'ENOENT') { | ||
| throw new Error(`Missing/invalid binary for wasm-opt [${wasmOptPath}] (do you need to specify WASM_OPT_BIN_PATH ?`); | ||
| } | ||
| throw err; | ||
| } | ||
|
|
There was a problem hiding this comment.
Use Optional Chaining to Prevent Runtime Errors
The static analysis tool suggests changing to an optional chain at line 179. Modify the condition to safely check for the existence of err.code before accessing it, preventing potential runtime errors if err is undefined.
Apply this diff to implement the optional chaining:
- if (err && err.code && err.code === 'ENOENT') {
+ if (err?.code === 'ENOENT') {
throw new Error(`Missing/invalid binary for wasm-opt [${wasmOptPath}] (do you need to specify WASM_OPT_BIN_PATH ?`);
}📝 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.
| const wasmOptPath = env.WASM_OPT_BIN_PATH ?? args?.wasmOptBinPath ?? fileURLToPath(import.meta.resolve('binaryen/bin/wasm-opt')); | |
| try { | |
| await stat(wasmOptPath); | |
| } catch (err) { | |
| if (err && err.code && err.code === 'ENOENT') { | |
| throw new Error(`Missing/invalid binary for wasm-opt [${wasmOptPath}] (do you need to specify WASM_OPT_BIN_PATH ?`); | |
| } | |
| throw err; | |
| } | |
| const wasmOptPath = env.WASM_OPT_BIN_PATH ?? args?.wasmOptBinPath ?? fileURLToPath(import.meta.resolve('binaryen/bin/wasm-opt')); | |
| try { | |
| await stat(wasmOptPath); | |
| } catch (err) { | |
| if (err?.code === 'ENOENT') { | |
| throw new Error(`Missing/invalid binary for wasm-opt [${wasmOptPath}] (do you need to specify WASM_OPT_BIN_PATH ?`); | |
| } | |
| throw err; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 179-179: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| read(len) { | ||
| if (this.#buffer.byteLength === 0 && this.#closed) throw { tag: 'closed' }; | ||
| const n = Number(len); |
There was a problem hiding this comment.
Handle Potential Loss of Precision When Converting len from BigInt to Number
In methods like read, skip, writeZeroes, blockingWriteZeroesAndFlush, splice, and blockingSplice, len can be a number or a bigint. Converting len to a Number using Number(len) may lead to loss of precision or unexpected behavior if len exceeds Number.MAX_SAFE_INTEGER (253 - 1).
Consider adding checks to ensure len is within the safe integer range or updating the logic to handle large bigint values without conversion to Number. This will prevent potential data corruption or runtime errors.
Also applies to: 64-66, 233-234, 241-242, 250-252, 262-264
| async blockingWriteAndFlush(contents) { | ||
| if (this.#readableController) { | ||
| this.#readableController?.enqueue(contents); | ||
| } else if (this.#prevWritePromise) { | ||
| throw new Error("waiting for previous write to finish"); | ||
| } else { | ||
| try { | ||
| await this.#writer.write(contents); | ||
| } catch (err) { | ||
| throw { tag: 'last-operation-failed', val: new IoError(err.toString()) }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure #writer is Initialized in blockingWriteAndFlush
In blockingWriteAndFlush, there is a possibility that this.#writer is undefined when this.#readableController and this.#prevWritePromise are falsy. This will lead to a runtime error when calling this.#writer.write(contents).
Apply this diff to address the issue:
} else {
+ if (!this.#writer) {
+ throw new Error("OutputStream writer is not initialized.");
+ }
try {
await this.#writer.write(contents);
} catch (err) {
throw { tag: 'last-operation-failed', val: new IoError(err.toString()) };
}
}📝 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.
| async blockingWriteAndFlush(contents) { | |
| if (this.#readableController) { | |
| this.#readableController?.enqueue(contents); | |
| } else if (this.#prevWritePromise) { | |
| throw new Error("waiting for previous write to finish"); | |
| } else { | |
| try { | |
| await this.#writer.write(contents); | |
| } catch (err) { | |
| throw { tag: 'last-operation-failed', val: new IoError(err.toString()) }; | |
| } | |
| } | |
| } | |
| async blockingWriteAndFlush(contents) { | |
| if (this.#readableController) { | |
| this.#readableController?.enqueue(contents); | |
| } else if (this.#prevWritePromise) { | |
| throw new Error("waiting for previous write to finish"); | |
| } else { | |
| if (!this.#writer) { | |
| throw new Error("OutputStream writer is not initialized."); | |
| } | |
| try { | |
| await this.#writer.write(contents); | |
| } catch (err) { | |
| throw { tag: 'last-operation-failed', val: new IoError(err.toString()) }; | |
| } | |
| } | |
| } |
| write: (contents) => { | ||
| // console.log() inserts a '\n' (which is 10) so try to skip that | ||
| if (contents[contents.length - 1] === 10) { | ||
| contents = contents.subarray(0, contents.length - 1); | ||
| } | ||
| console.log(textDecoder.decode(contents)); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicated code in stdout and stderr stream handling.
The content handling logic is duplicated between stdout and stderr streams.
Extract the common logic into a shared function:
+const handleStreamContents = (contents) => {
+ // console.log() inserts a '\n' (which is 10) so try to skip that
+ if (contents[contents.length - 1] === 10) {
+ contents = contents.subarray(0, contents.length - 1);
+ }
+ return contents;
+};
// In stdout.getStdout()
- if (contents[contents.length - 1] === 10) {
- contents = contents.subarray(0, contents.length - 1);
- }
- console.log(textDecoder.decode(contents));
+ console.log(textDecoder.decode(handleStreamContents(contents)));
// In stderr.getStderr()
- if (contents[contents.length - 1] === 10) {
- contents = contents.subarray(0, contents.length - 1);
- }
- console.error(textDecoder.decode(contents));
+ console.error(textDecoder.decode(handleStreamContents(contents)));Also applies to: 87-93
| _args = args; | ||
| } | ||
| export function _setCwd (cwd) { | ||
| fsSetCwd(_cwd = cwd); |
There was a problem hiding this comment.
Fix the assignment in expression.
The assignment within the expression can lead to confusion and is flagged by static analysis. Consider separating the assignment and the function call.
Apply this diff to fix the issue:
- fsSetCwd(_cwd = cwd);
+ _cwd = cwd;
+ fsSetCwd(_cwd);📝 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.
| fsSetCwd(_cwd = cwd); | |
| _cwd = cwd; | |
| fsSetCwd(_cwd); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| test("Transpile async (asyncify)", async () => { | ||
| const { instance, cleanup } = 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", | ||
| ], | ||
| }, | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| strictEqual(instance.runSync instanceof AsyncFunction, false, "runSync() should be a sync function"); | ||
| strictEqual(instance.runAsync instanceof AsyncFunction, true, "runAsync() should be an async function"); | ||
|
|
||
| strictEqual(instance.runSync(), "called sync"); | ||
| strictEqual(await instance.runAsync(), "called async"); | ||
|
|
||
| await cleanup(); | ||
| }); |
There was a problem hiding this comment.
Remove duplicate test case.
This test case is an exact duplicate of the previous asyncify test at lines 108-142.
| // TODO: Transpiled modules expect host mappings like `wasi:cli/stderr` | ||
| // There has to be a better way than having to spell out every single one. | ||
| // |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comment about host mappings.
The TODO comment indicates a potential maintenance issue with host mappings. Consider creating a helper function or configuration file to manage these mappings more efficiently.
Would you like me to help design a more maintainable solution for managing host mappings?
|
|
||
| // 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 template literal bug in ok() function.
There's a bug in the ok() function where it uses ${err} instead of ${msg}.
-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}`; } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/preview2-shim/lib/browser-async/io/streams.js (2)
44-70:⚠️ Potential issueHandle potential loss of precision in bigint to number conversion.
Converting
lentoNumbermay lead to precision loss for values larger thanNumber.MAX_SAFE_INTEGER.read(len) { if (this.#buffer.byteLength === 0 && this.#closed) throw { tag: 'closed' }; + if (typeof len === 'bigint' && len > BigInt(Number.MAX_SAFE_INTEGER)) { + throw new Error('Length exceeds maximum safe integer value'); + } const n = Number(len);
175-209:⚠️ Potential issueAdd null check for writer before write operations.
Both
writeandblockingWriteAndFlushmethods may attempt to use an undefined writer.write(contents) { if (this.#closed) throw { tag: 'closed' }; if (this.#readableController) { this.#readableController?.enqueue(contents); } else if (this.#prevWritePromise) { throw new Error("waiting for previous write to finish"); + } else if (!this.#writer) { + throw new Error("OutputStream writer is not initialized"); } else { this.#prevWritePromise = this.#writer.write(contents).then(
🧹 Nitpick comments (3)
packages/preview2-shim/lib/browser-async/io/streams.js (3)
22-38: Consider adding a timeout mechanism to prevent infinite loops.The
#fillBuffermethod could potentially loop indefinitely if the stream continuously returns empty buffers. Consider implementing a timeout mechanism or maximum retry count.async #fillBuffer() { if (this.#buffer.byteLength === 0) { + let retryCount = 0; + const MAX_RETRIES = 1000; // Adjust based on requirements while (true) { + if (retryCount++ > MAX_RETRIES) { + throw { tag: 'last-operation-failed', val: new IoError('Maximum retry attempts exceeded') }; + } const { value, done } = await this.#reader.read();
258-280: Enhance error handling in splice operations.The splice operations should handle potential errors from
checkWriteand invalid input parameters.splice(src, len) { + if (!src) { + throw new Error("Source stream is required"); + } + if (typeof len !== 'number' && typeof len !== 'bigint') { + throw new Error("Length must be a number or bigint"); + } const n = this.checkWrite(); const contents = src.read(Number(len) < n ? len : n); this.write(contents);
1-281: Consider implementing retry and timeout strategies.The implementation would benefit from a more robust error handling strategy:
- Add configurable timeout mechanisms for blocking operations
- Implement retry strategies with exponential backoff for transient failures
- Consider adding event emitters for error monitoring
Would you like me to provide a detailed implementation for any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/preview2-shim/lib/browser-async/io/poll.js(1 hunks)packages/preview2-shim/lib/browser-async/io/streams.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/preview2-shim/lib/browser-async/io/poll.js
🔇 Additional comments (2)
packages/preview2-shim/lib/browser-async/io/streams.js (2)
1-20: LGTM! Well-structured initialization with proper error handling.The imports and class initialization are well-implemented with appropriate use of private fields and proper handling of optional streams.
113-147: LGTM! Well-implemented stream initialization.The OutputStream implementation properly handles both WritableStream and internal ReadableStream cases with appropriate configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
src/cmd/componentize.js (2)
18-18:⚠️ Potential issueReplace eval with direct dynamic import
Usingeval('import(...)')for dynamic imports poses potential security risks and may degrade performance.- const { componentize: componentizeFn } = await eval('import("@bytecodealliance/componentize-js")'); + const { componentize: componentizeFn } = await import("@bytecodealliance/componentize-js");
23-24: 🛠️ Refactor suggestionAdd error handling around file operations
Reading the source without a try-catch or equivalent might cause unhandled promise rejections if the file doesn’t exist or is unreadable.- const source = await readFile(sourcePath, 'utf8'); + let source; + try { + source = await readFile(sourcePath, 'utf8'); + } catch (err) { + throw new Error(`Failed to read source file: ${err.message}`); + }packages/preview2-shim/lib/browser-async/clocks/monotonic-clock.js (1)
59-66: 🛠️ Refactor suggestionEnforce input type for subscribeDuration
Same as a past suggestion, ensure that thewhenargument is abigintto avoid unexpected runtime errors, especially if types are not checked upstream.export const subscribeDuration = (when) => { + if (typeof when !== 'bigint') { + throw new TypeError('Duration must be a BigInt'); + } if (when < 0) return new Pollable(); return new Pollable( new Promise((resolve) => {packages/preview2-shim/lib/browser-async/cli.js (3)
15-15:⚠️ Potential issueFix the assignment in expression.
The assignment within the expression can lead to confusion and is flagged by static analysis.
Apply this diff to fix the issue:
- fsSetCwd(_cwd = cwd); + _cwd = cwd; + fsSetCwd(_cwd);
67-73: 🛠️ Refactor suggestionRefactor duplicated stream handling logic.
The content handling logic is duplicated between stdout and stderr streams.
Extract the common logic into a shared function:
+const handleStreamContents = (contents) => { + if (contents[contents.length - 1] === 10) { + contents = contents.subarray(0, contents.length - 1); + } + return contents; +}; - if (contents[contents.length - 1] === 10) { - contents = contents.subarray(0, contents.length - 1); - } - console.log(textDecoder.decode(contents)); + console.log(textDecoder.decode(handleStreamContents(contents))); - if (contents[contents.length - 1] === 10) { - contents = contents.subarray(0, contents.length - 1); - } - console.error(textDecoder.decode(contents)); + console.error(textDecoder.decode(handleStreamContents(contents)));Also applies to: 87-93
101-102:⚠️ Potential issueImplement the terminal classes.
The
TerminalInputandTerminalOutputclasses are empty but exported and used.Would you like me to help generate the implementation for these classes?
src/cmd/opt.js (1)
189-191:⚠️ Potential issueAdd retry limit to prevent infinite recursion.
The recursive call to
wasmOptin the error handler could lead to infinite recursion.Apply this diff to add a retry limit:
+ let retryCount = 0; + const MAX_RETRIES = 3; + async function wasmOpt(args) { + if (retryCount >= MAX_RETRIES) { + throw new Error('Maximum retry attempts exceeded for wasm-opt'); + } try { // ... existing code ... } catch (e) { if (e.toString().includes('BasicBlock requested')) { + retryCount++; return wasmOpt(args); } throw e; } }
🧹 Nitpick comments (20)
packages/preview2-shim/lib/browser-async/filesystem.js (2)
17-19: Return parsed data to enhance usability
Currently,_getFileData()returns a JSON string which consumers will need to parse again. Consider returning an already-parsed object instead to reduce overhead and simplify usage.
112-126: Finish or remove unimplemented write logic
ThewriteViaStream()method is unimplemented and permanently throws'unimplemented'. If this functionality is needed, complete the implementation. Otherwise, consider removing this method.test/browser/preview2.js (1)
166-254: Consider splitting the large test for maintainability
The[async] guest http/incoming-handler exporttest case includes multiple steps and configurations. Splitting it into smaller tests or helper methods could enhance readability and troubleshooting.packages/preview2-shim/lib/browser-async/http/types.js (2)
3-229: Remove or clarify large commented-out WASI references
Consider removing these large blocks of commented-out code if no longer used, or convert them into official documentation to improve readability.
761-761: Standardize error reporting
Currently,this.#erroris assigned a freeform internal-error object witherr.toString(). Consider a unified error format or code to keep error handling consistent across the module.test/fixtures/wasi/0.2.2/wasi_filesystem@0.2.2.wit (4)
3-26: Consider clarifying path encoding details in the docstringAlthough the documentation states that paths are treated as Unicode Scalar Values (USVs) and must use forward-slash (
/), it may be beneficial to include how non-UTF-8 encodings or other edge cases should be handled on platforms that traditionally support non-UTF-8 file names.
27-28: Align version annotations with potential future releasesThe
@since(version = 0.2.0)annotations are helpful. However, consider referencing a stable WASI release or explicitly highlighting how future major/minor changes will be tracked to maintain clarity for users migrating from earlier versions.
510-525: Consider salting metadata-hash output to enhance privacyThe documentation indicates that the metadata hash may include a secret value. For better privacy and security, ensure this secret is adequately randomized and frequently regenerated to prevent third-party correlation or replay attacks on the returned hash.
560-567: Restrict privilege of preopened directoriesProviding preopened directories can lead to inadvertent privilege escalation if the base directory is too permissive. Consider confirming that only minimal and necessary directories are exposed, and add validation or documentation emphasizing how preopens are selected for a given environment.
src/cmd/componentize.js (1)
36-38: Optional silent vs. verbose modes
The condition foropts.quietis a good start for controlling console output. If the workflow demands more elaborate logs or levels (e.g., debug/info/warn), consider a more flexible logging approach rather than a single boolean flag.packages/preview2-shim/lib/browser-async/clocks/monotonic-clock.js (1)
48-48: Revisit naming for subscribeInstant
"subscribeInstant" calls "subscribeDuration(when - now())", which is intuitive from an implementation standpoint but slightly hides the concept that "subscribeInstant" is effectively scheduling a future poll. Consider adding clarifying comments or an additional docstring note about potential negative durations when the target time is in the past.package.json (1)
29-33: Consider adding browser field for conditional exports.The imports field configures browser/default resolution for ora, but the exports field doesn't have browser-specific configurations. Consider adding a browser field in exports for consistent conditional resolution.
"imports": { "#ora": { "browser": "./src/ora-shim.js", "default": "ora" } }, "exports": { ".": { + "browser": "./src/browser.js", "default": "./src/api.js" } }test/browser/webidl.js (1)
Line range hint
36-58: Improve error handling in the server request handler.The server request handler could benefit from more robust error handling:
- The URL construction could throw if the request URL is malformed
- The error response should include proper content-type headers
Apply this diff to enhance error handling:
if (req.url.startsWith('/tmpdir/')) { fileUrl = new URL(`.${req.url.slice(7)}`, outDirUrl); } else { - fileUrl = new URL(`../../${req.url}`, import.meta.url); + try { + fileUrl = new URL(`../../${req.url}`, import.meta.url); + } catch (e) { + res.writeHead(400, { 'content-type': 'text/plain' }); + res.end('Invalid URL'); + return; + } } try { const html = await readFile(fileUrl); res.writeHead(200, { 'content-type': mime.getType(extname(req.url)) }); res.end(html); } catch (e) { if (e.code === 'ENOENT') { - res.writeHead(404); + res.writeHead(404, { 'content-type': 'text/plain' }); res.end(e.message); } else { - res.writeHead(500); + res.writeHead(500, { 'content-type': 'text/plain' }); res.end(e.message); } }src/cmd/opt.js (1)
74-86: Track TODO comments for future improvements.Multiple TODO comments indicate pending improvements:
- Pre-asyncify builds of starling-monkey.wasm
- Custom starling-monkey.wasm option
- Option to skip wasm-opt with pre-asyncified builds
- Pre-optimized build of SM
Would you like me to create issues to track these TODO items?
test/helpers.js (1)
87-104: Add missing JSDoc param descriptions.The JSDoc comment is missing descriptions for several parameters in the
setupAsyncTestfunction, such asargs.jco,args.component.js.source, etc. Complete documentation helps with maintainability.src/cmd/transpile.js (1)
205-210: Enhance comment clarity for preoptimized and asyncify conditions.While the comment explains the basic conditions, it could be more explicit about:
- When preoptimization is used
- Why asyncify requires optimization
- The implications of each condition
Apply this diff to improve the comment:
- // 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 required in two cases: + // 1. When explicitly requested via opts.optimize + // 2. When using asyncify mode and the component is not preoptimized + // + // Preoptimized flag: + // - Used internally to indicate the component was built with optimized code + // - Not available as a CLI option + // - Helps avoid redundant optimization when using asyncify + // + // Asyncify requires optimization to ensure proper async/sync state managementcrates/js-component-bindgen/src/ts_bindgen.rs (1)
Line range hint
780-914: Optimize string concatenation in ts_func.The current implementation uses multiple string concatenations which could be inefficient for large codebases.
Consider using a string builder pattern or format! macro for more efficient string handling. For example:
// Instead of multiple format! calls and string concatenations let declaration = match func.kind { FunctionKind::Freestanding => { if is_js_identifier(&out_name) { format!("export {maybe_async}function {out_name}") } else { let (local_name, _) = iface.local_names.get_or_create(&out_name, &out_name); format!("export {{ {local_name} as {out_name} }};\ndeclare {maybe_async}function {local_name}") } } // ... rest of the match cases }; iface.src.push_str(&declaration);crates/js-component-bindgen/src/transpile_bindgen.rs (3)
156-168: Consider extracting the default values into a constant.While the async mode handling is correct, the default values for
async_importsandasync_exportscould be extracted into a constant for better maintainability.+const DEFAULT_ASYNC_CONFIG: (bool, HashSet<String>, HashSet<String>) = (false, Default::default(), Default::default()); let (use_asyncify, async_imports, async_exports) = match opts.async_mode.clone() { - None | Some(AsyncMode::Sync) => (false, Default::default(), Default::default()), + None | Some(AsyncMode::Sync) => DEFAULT_ASYNC_CONFIG, Some(AsyncMode::JavaScriptPromiseIntegration { imports, exports }) => ( false, imports.into_iter().collect(), exports.into_iter().collect(), ),
1096-1101: Reduce code duplication in instantiate selection.The async mode check and intrinsic selection is duplicated. Consider extracting this logic into a helper method.
+impl<'a> Instantiator<'a, '_> { + fn get_instantiate_intrinsic(&self) -> String { + if self.use_asyncify { + self.gen.intrinsic(match self.gen.opts.instantiation { + Some(InstantiationMode::Sync) => Intrinsic::AsyncifySyncInstantiate, + _ => Intrinsic::AsyncifyAsyncInstantiate, + }) + } else { + self.gen.intrinsic(Intrinsic::InstantiateCore) + } + } +} -instantiate = if self.use_asyncify { - self.gen.intrinsic(Intrinsic::AsyncifyAsyncInstantiate) -} else { - instantiate -}, +instantiate = self.get_instantiate_intrinsic(),Also applies to: 1109-1114
2069-2086: Improve variable naming for better code clarity.The variable
maybe_asynccould be renamed to better reflect its purpose, and the core export function handling could be more clearly structured.-let maybe_async = if is_async { "async " } else { "" }; +let async_prefix = if is_async { "async " } else { "" }; -let core_export_fn = self.core_def(def); -let callee = match self +let core_export_name = self.core_def(def); +let callee_name = match self .gen .local_names - .get_or_create(&core_export_fn, &core_export_fn) + .get_or_create(&core_export_name, &core_export_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/fixtures/components/async_call.component.wasmis excluded by!**/*.wasm
📒 Files selected for processing (62)
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)crates/wasm-tools-component/src/lib.rs(1 hunks)docs/src/contributing.md(1 hunks)docs/src/transpiling.md(1 hunks)package.json(3 hunks)packages/preview2-shim/lib/browser-async/cli.js(1 hunks)packages/preview2-shim/lib/browser-async/clocks.js(1 hunks)packages/preview2-shim/lib/browser-async/clocks/monotonic-clock.js(1 hunks)packages/preview2-shim/lib/browser-async/clocks/timezone.js(1 hunks)packages/preview2-shim/lib/browser-async/clocks/wall-clock.js(1 hunks)packages/preview2-shim/lib/browser-async/filesystem.js(1 hunks)packages/preview2-shim/lib/browser-async/http.js(1 hunks)packages/preview2-shim/lib/browser-async/http/incoming-handler.js(1 hunks)packages/preview2-shim/lib/browser-async/http/outgoing-handler.js(1 hunks)packages/preview2-shim/lib/browser-async/http/types.js(1 hunks)packages/preview2-shim/lib/browser-async/index.js(1 hunks)packages/preview2-shim/lib/browser-async/io.js(1 hunks)packages/preview2-shim/lib/browser-async/io/error.js(1 hunks)packages/preview2-shim/lib/browser-async/io/poll.js(1 hunks)packages/preview2-shim/lib/browser-async/io/streams.js(1 hunks)packages/preview2-shim/lib/browser-async/random.js(1 hunks)packages/preview2-shim/lib/browser-async/random/insecure.js(1 hunks)packages/preview2-shim/lib/browser-async/random/random.js(1 hunks)packages/preview2-shim/lib/browser-async/sockets.js(1 hunks)packages/preview2-shim/lib/browser/cli.js(1 hunks)packages/preview2-shim/lib/browser/random.js(1 hunks)packages/preview2-shim/package.json(1 hunks)src/api.js(1 hunks)src/cmd/componentize.js(1 hunks)src/cmd/opt.js(5 hunks)src/cmd/transpile.js(8 hunks)src/common.js(2 hunks)src/jco.js(3 hunks)test/async.js(1 hunks)test/browser.html(0 hunks)test/browser/browser-preview2.html(1 hunks)test/browser/preview2.js(1 hunks)test/browser/test.js(1 hunks)test/browser/webidl.js(6 hunks)test/cli.js(33 hunks)test/codegen.js(1 hunks)test/fixtures/browser/test-pages/wasi-http-incoming-handler.guest-export.async.preview2.html(1 hunks)test/fixtures/components/js/browser-incoming-handler/component.js(1 hunks)test/fixtures/components/js/browser-incoming-handler/component.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_cli@0.2.2.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_clocks@0.2.2.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_filesystem@0.2.2.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_http@0.2.2.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_io@0.2.2.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_random@0.2.2.wit(1 hunks)test/fixtures/wasi/0.2.2/wasi_sockets@0.2.2.wit(1 hunks)test/helpers.js(3 hunks)test/test.js(4 hunks)xtask/src/build/jco.rs(1 hunks)xtask/src/generate/wasi_types.rs(1 hunks)
💤 Files with no reviewable changes (2)
- crates/js-component-bindgen/src/core.rs
- test/browser.html
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/wasi/0.2.2/wasi_random@0.2.2.wit
🚧 Files skipped from review as they are similar to previous changes (37)
- packages/preview2-shim/lib/browser/random.js
- packages/preview2-shim/lib/browser/cli.js
- packages/preview2-shim/lib/browser-async/http/outgoing-handler.js
- crates/js-component-bindgen/src/lib.rs
- packages/preview2-shim/lib/browser-async/random.js
- test/fixtures/components/js/browser-incoming-handler/component.wit
- test/browser/test.js
- test/fixtures/components/js/browser-incoming-handler/component.js
- src/api.js
- xtask/src/generate/wasi_types.rs
- packages/preview2-shim/lib/browser-async/http.js
- crates/js-component-bindgen/src/function_bindgen.rs
- packages/preview2-shim/lib/browser-async/index.js
- packages/preview2-shim/lib/browser-async/http/incoming-handler.js
- packages/preview2-shim/lib/browser-async/clocks/wall-clock.js
- test/codegen.js
- docs/src/contributing.md
- packages/preview2-shim/package.json
- packages/preview2-shim/lib/browser-async/clocks.js
- test/test.js
- docs/src/transpiling.md
- packages/preview2-shim/lib/browser-async/io/streams.js
- packages/preview2-shim/lib/browser-async/random/random.js
- test/fixtures/browser/test-pages/wasi-http-incoming-handler.guest-export.async.preview2.html
- xtask/src/build/jco.rs
- crates/wasm-tools-component/src/lib.rs
- packages/preview2-shim/lib/browser-async/io/error.js
- packages/preview2-shim/lib/browser-async/io.js
- test/browser/browser-preview2.html
- test/async.js
- packages/preview2-shim/lib/browser-async/clocks/timezone.js
- test/fixtures/wasi/0.2.2/wasi_http@0.2.2.wit
- packages/preview2-shim/lib/browser-async/random/insecure.js
- test/fixtures/wasi/0.2.2/wasi_cli@0.2.2.wit
- packages/preview2-shim/lib/browser-async/sockets.js
- test/fixtures/wasi/0.2.2/wasi_sockets@0.2.2.wit
- packages/preview2-shim/lib/browser-async/io/poll.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Jco Build
🔇 Additional comments (39)
packages/preview2-shim/lib/browser-async/filesystem.js (4)
1-3: Imports look good
No immediate concerns with these import statements.
164-168: Handle Non-Zero Offset Appropriately inwriteMethod
226-228: Implement unimplemented filesystem methods or throw explicit errorsAlso applies to: 230-232, 239-241, 243-245, 247-249, 251-253, 255-257
275-276: Avoid usingdeleteoperator on prototype propertiestest/browser/preview2.js (1)
99-121: Sanitize 'req.url' to prevent directory traversalpackages/preview2-shim/lib/browser-async/http/types.js (1)
293-299: Handle header value encoding more carefullytest/fixtures/wasi/0.2.2/wasi_filesystem@0.2.2.wit (1)
405-405: Prevent race conditions when reading directory entriesMultiple streams may be active on the same directory, risking possible race conditions if the underlying filesystem changes during reads. Depending on your user requirements, you may want to ensure concurrency control or clarify the expected behavior if files are added/removed during iteration.
package.json (1)
54-54: Verify experimental flag compatibility.The test script uses the experimental
--experimental-wasm-jspiflag. This flag is required for JavaScript Promise Integration (JSPI) support but may not be available in all Node.js versions.✅ Verification successful
JSPI flag compatibility verified
The
--experimental-wasm-jspiflag is fully supported in the current Node.js environment (v22.9.0). The test script configuration is valid and working as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check Node.js version compatibility for --experimental-wasm-jspi flag node --version node --experimental-wasm-jspi --eval "console.log('JSPI support available')" || echo "JSPI not supported"Length of output: 132
src/common.js (2)
80-95: Great documentation improvement!The added JSDoc clearly describes the function's purpose, expected command format, and parameters.
125-127: Good defensive programming!The added null check before cleanup prevents potential errors when tmpDir creation fails.
test/fixtures/wasi/0.2.2/wasi_clocks@0.2.2.wit (1)
1-103: Well-structured and documented interface definitions!The WASI clocks interfaces are thoroughly documented with clear explanations of:
- Purpose and portability
- Monotonic vs wall clock behavior
- Time representation and resolution
- Function behaviors and constraints
crates/js-component-bindgen-component/wit/js-component-bindgen.wit (2)
69-71: Well-designed async mode configuration!The async-mode variant provides clear options for handling async operations:
- sync: default synchronous mode
- jspi: JavaScript Promise Integration
- asyncify: Asyncify transformation
Also applies to: 79-85
74-77: Consider adding validation for imports/exports lists.The async-imports-exports record should validate that the provided import/export names exist in the component.
test/browser/webidl.js (2)
1-15: LGTM! Import statements and function rename are well organized.The imports are logically grouped, and the function rename from
browserTesttobrowserWebIdlTestimproves clarity by being more specific about its purpose.
79-83: LGTM! Consistent use of object parameter pattern.The refactoring to use object parameters in
testBrowserPagecalls improves code maintainability and readability by making the parameter roles explicit.Also applies to: 118-122, 156-160
crates/js-component-bindgen-component/src/lib.rs (2)
56-68: LGTM! Clean and exhaustive From implementation.The
Fromtrait implementation forAsyncModeis well-structured and handles all variants correctly:
Sync→SyncJspi→JavaScriptPromiseIntegrationAsyncify→Asyncify
93-93: LGTM! Consistent async_mode field addition.The
async_modefield is consistently added to bothgenerateandgenerate_typesmethods with proper type conversion usingInto::into.Also applies to: 180-180
src/cmd/opt.js (2)
77-78: LGTM! Clean async mode integration.The integration of async mode in the optimization arguments is clean and straightforward.
179-183: Use optional chaining for error code check.The error code check can be simplified using optional chaining.
Apply this diff as suggested in the previous review:
- if (err && err.code && err.code === 'ENOENT') { + if (err?.code === 'ENOENT') { throw new Error(`Missing/invalid binary for wasm-opt [${wasmOptPath}] (do you need to specify WASM_OPT_BIN_PATH ?`); }src/jco.js (3)
37-37: LGTM! Clear engine customization option.The
--engineoption is well-documented and provides clear functionality for custom engine specification.
56-60: LGTM! Well-structured async mode options.The async-related options are well-organized with:
- Clear experimental status warning
- Descriptive examples in help text
- Consistent defaults
85-89: LGTM! Consistent async options in types command.The async options are consistently replicated in the types command, maintaining API uniformity.
test/fixtures/wasi/0.2.2/wasi_io@0.2.2.wit (4)
1-34: LGTM! Well-documented error interface.The error interface is well-designed with:
- Clear documentation on usage
- Flexible downcast mechanism
- Proper versioning annotations
36-78: LGTM! Robust poll interface design.The poll interface provides a solid foundation for async I/O with:
- Clear documentation of error cases
- Proper resource cleanup hints
- Well-defined blocking behavior
80-321: LGTM! Comprehensive streams interface.The streams interface is exceptionally well-designed with:
- Detailed documentation for all methods
- Clear error handling patterns
- Proper resource lifecycle management
- Efficient non-blocking I/O support
323-331: LGTM! Clean world imports definition.The world imports section properly aggregates all interfaces with consistent versioning.
test/helpers.js (2)
1-31: LGTM! Well-organized imports and constants.The imports are properly organized using the recommended
node:prefix, and the default async constants provide a good set of WASI interface defaults.
454-464: LGTM! Clean implementation of getRandomPort.The function properly creates a temporary server, gets its port, and ensures cleanup through the 'close' event.
test/cli.js (2)
118-190: LGTM! Comprehensive async test coverage.The test cases thoroughly verify:
- JSPI support with proper feature detection
- Asyncify functionality
- Both sync and async function behavior
- Function types and results
641-669: Cache key generation could lead to collisions.The current cache key is generated using
args.join("+"), which may lead to collisions if the arguments contain+characters.src/cmd/transpile.js (1)
17-31: LGTM! Well-defined default async operations.The constants provide a comprehensive set of default async operations for WASI interfaces, with clear naming and paths.
crates/js-component-bindgen/src/intrinsics.rs (2)
8-11: LGTM! Well-structured async intrinsics.The new intrinsic variants are well-named and provide comprehensive coverage for async operations:
- AsyncifyAsyncInstantiate
- AsyncifySyncInstantiate
- AsyncifyWrapExport
- AsyncifyWrapImport
212-231: Missing return statement in asyncifyWrapImport function.The function doesn't return a value after setting
asyncifyPromise, which can lead to unexpectedundefinedresults.crates/js-component-bindgen/src/ts_bindgen.rs (1)
36-37: LGTM! Well-designed async tracking.The
async_importsandasync_exportsfields use HashSet for efficient lookup and are properly integrated into the TsBindgen struct.crates/js-component-bindgen/src/transpile_bindgen.rs (5)
14-14: LGTM: Clean addition of async support configuration.The addition of
HashSetimport and theasync_modefield toTranspileOptsis well-structured and provides a clear way to configure async behavior.Also applies to: 73-75
78-89: LGTM: Well-structured AsyncMode enum.The
AsyncModeenum provides a clear and type-safe way to configure async behavior with appropriate variants for different async strategies (JSPI and Asyncify).
138-140: LGTM: Proper tracking of async state in JsBindgen.The addition of
all_core_exported_funcsanduse_asyncifyfields provides necessary state tracking for async function handling.Also applies to: 142-142
2054-2067:⚠️ Potential issueHandle potential panic in export name slicing.
Similar to the import handling, the use of
unwrap()onget(0..i)could panic. Use a safer string manipulation approach.-export_name.find('@') - .map(|i| { - self.async_exports.contains(&format!( - "{}#{}", - export_name.get(0..i).unwrap(), - func.name - )) - }) +export_name.split('@') + .next() + .map(|base_name| { + self.async_exports.contains(&format!( + "{}#{}", + base_name, + func.name + )) + })Likely invalid or redundant comment.
1185-1194:⚠️ Potential issueHandle potential panic in import name slicing.
The use of
unwrap()onget(0..i)could panic ifiis out of bounds. Consider using a safer approach.- .contains(&format!("{}#{func_name}", import_name.get(0..i).unwrap())) + .contains(&format!("{}#{func_name}", import_name.get(0..i).unwrap_or(import_name)))Alternatively, use string splitting which is safer:
-import_name.find('@') - .map(|i| { - self.async_imports - .contains(&format!("{}#{func_name}", import_name.get(0..i).unwrap())) - }) +import_name.split('@') + .next() + .map(|base_name| { + self.async_imports + .contains(&format!("{}#{func_name}", base_name)) + })Likely invalid or redundant comment.
| segmentIdx = subpath.indexOf('/'); | ||
| const segment = segmentIdx === -1 ? subpath : subpath.slice(0, segmentIdx); | ||
| if (segment === '..') throw 'no-entry'; | ||
| if (segment === '.' || segment === ''); |
There was a problem hiding this comment.
Fix suspicious empty if statement
if (segment === '.' || segment === ''); is a no-op that likely breaks path handling. Correct it to handle segment === '' properly.
- if (segment === '.' || segment === '');
+ if (segment === '.' || segment === '') {
+ // do nothing
+ }📝 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 (segment === '.' || segment === ''); | |
| if (segment === '.' || segment === '') { | |
| // do nothing | |
| } |
| let cleanup = async () => { | ||
| if (componentBuildCleanup) { | ||
| try { | ||
| await componentBuildCleanup(); | ||
| } catch {} | ||
| } | ||
| try { | ||
| await rm(outputDir, { recursive: true }); | ||
| } catch {} | ||
| }; |
There was a problem hiding this comment.
Fix potential memory leak in cleanup function.
The rm function is used but not imported from node:fs/promises. This could lead to a runtime error.
Apply this diff to fix the issue:
-import { cp, mkdtemp, writeFile, stat, mkdir, readFile } from "node:fs/promises";
+import { cp, mkdtemp, writeFile, stat, mkdir, readFile, rm } from "node:fs/promises";Committable suggestion skipped: line range outside the PR's diff.
| while (bodyHTML === "<h1>Running</h1>") { | ||
| bodyHTML = await body.evaluate((el) => el.innerHTML); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout to prevent potential infinite loop.
The while loop checking for bodyHTML === "<h1>Running</h1>" could potentially run indefinitely if the page never updates.
Apply this diff to add a timeout:
+ const startTime = Date.now();
+ const TIMEOUT = 30000; // 30 seconds timeout
while (bodyHTML === "<h1>Running</h1>") {
+ if (Date.now() - startTime > TIMEOUT) {
+ throw new Error('Timeout waiting for page to update from "Running" state');
+ }
bodyHTML = await body.evaluate((el) => el.innerHTML);
+ await new Promise(resolve => setTimeout(resolve, 100)); // Add small delay to prevent CPU spinning
}📝 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 (bodyHTML == "<h1>Running</h1>") { | |
| while (bodyHTML === "<h1>Running</h1>") { | |
| bodyHTML = await body.evaluate((el) => el.innerHTML); | |
| } | |
| } | |
| if (bodyHTML == "<h1>Running</h1>") { | |
| const startTime = Date.now(); | |
| const TIMEOUT = 30000; // 30 seconds timeout | |
| while (bodyHTML === "<h1>Running</h1>") { | |
| if (Date.now() - startTime > TIMEOUT) { | |
| throw new Error('Timeout waiting for page to update from "Running" state'); | |
| } | |
| bodyHTML = await body.evaluate((el) => el.innerHTML); | |
| await new Promise(resolve => setTimeout(resolve, 100)); // Add small delay to prevent CPU spinning | |
| } | |
| } |
1 participant
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Testing