feat: support more ways to load wasm on browser
#114
Conversation
🦋 Changeset detectedLatest commit: 56ab004 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces a feature for loading WebAssembly (WASM) in browser environments with additional methods. Documentation has been updated with new API subsections for both Node and browser environments and benchmark results reflecting performance improvements. The module’s exports now include a browser-specific entry. Core processor logic has been modified to accept either a WASM instance or a source, with corresponding type definitions added. Configuration adjustments for initial memory allocation have been made, and test files have been updated to use the promise-based file system API alongside a new test suite for processor functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Processor
participant WASM
Client->>Processor: Call getProcessor(getWasm)
alt getWasm returns WASM Instance
Processor->>WASM: Use instance directly
else getWasm returns WASM Source
Processor->>Processor: Convert source to ArrayBuffer
Processor->>WASM: Instantiate WASM module
end
WASM-->>Processor: Return WASM Instance
Processor-->>Client: Return Processor instance
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/fixtures.tsOops! Something went wrong! :( ESLint: 9.24.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js eslint.config.jsOops! Something went wrong! :( ESLint: 9.24.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js test/fixtures.spec.tsOops! Something went wrong! :( ESLint: 9.24.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (2)
✨ 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 (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Copilot reviewed 6 out of 9 changed files in this pull request and generated 1 comment.
Files not reviewed (3)
- benchmark/benchmark.txt: Language not supported
- package.json: Language not supported
- target.json: Language not supported
Comments suppressed due to low confidence (1)
src/processor.ts:186
- Using non-null assertions for encoder and decoder assumes they are always initialized by this point. It would be safer to ensure they are properly initialized beforehand or refactor the initialization logic to avoid using the non-null operator.
const filePath = encoder!.encode(filepath)
wasm on browser
commit: |
size-limit report 📦
|
📊 Package size report 0.2%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 96.25% 96.38% +0.13%
==========================================
Files 4 4
Lines 80 83 +3
Branches 32 36 +4
==========================================
+ Hits 77 80 +3
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/processor.ts (2)
113-119:⚠️ Potential issueRelying on function arity to differentiate between source and instance providers is fragile
Using
getWasm.length === 0to determine if the function is a source provider or instance provider can lead to unexpected behavior if a zero-argument function is provided that returns an instance.Consider using an explicit configuration option or type discrimination to differentiate between the two types:
-export const getProcessor = ( - getWasm: GetWebAssemblyInstance | GetWebAssemblySource, -) => { +export const getProcessor = ( + getWasm: GetWebAssemblyInstance | GetWebAssemblySource, + options?: { isInstance?: boolean }, +) => { // ... - if (!wasmBufferSource && !wasmBufferSourcePromise && getWasm.length === 0) { + if (!wasmBufferSource && !wasmBufferSourcePromise && !options?.isInstance) { wasmBufferSourcePromise = Promise.resolve( (getWasm as GetWebAssemblySource)(), ).then(source =>
137-144:⚠️ Potential issueSame issue with function arity in WASM instantiation
The same fragile pattern of using
getWasm.length === 0appears here as well. This should be refactored along with the earlier instance.- const wasm = - getWasm.length === 0 - ? await WebAssembly.instantiate(wasmBufferSource!, go.importObject) - : { - instance: await (getWasm as GetWebAssemblyInstance)( - go.importObject, - ), - } + const wasm = !options?.isInstance + ? await WebAssembly.instantiate(wasmBufferSource!, go.importObject) + : { + instance: await (getWasm as GetWebAssemblyInstance)( + go.importObject, + ), + }
🧹 Nitpick comments (1)
test/processor.spec.ts (1)
39-54: Consider expanding test coverage with more specific test casesThe current test reads all fixtures and tests them against snapshots, which is good for regression testing. Consider adding more specific test cases that verify particular aspects of the functionality (e.g., error handling, different input formats, edge cases).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
main.wasmis excluded by!**/*.wasmtest/__snapshots__/processor.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
.changeset/witty-actors-dress.md(1 hunks)README.md(4 hunks)benchmark/benchmark.txt(1 hunks)package.json(1 hunks)src/processor.ts(5 hunks)target.json(1 hunks)test/fixtures.spec.ts(1 hunks)test/processor.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
test/processor.spec.ts (3)
src/processor.ts (2)
GetWebAssemblyInstance(32-34)getProcessor(39-267)src/index.ts (3)
processor(16-18)parse(20-21)src/types.ts (2)
ShOptions(160-165)ShPrintOptions(167-169)
🪛 LanguageTool
benchmark/benchmark.txt
[grammar] ~15-~15: Make sure that the singular noun after the number ‘312.45’ is correct.
Context: ... (291.31 kb … 312.45 kb) 292.09 kb ████▇▇▇▇▁▁▁▁▇▁██▇████ ...
(CD_POINT_CD_NN)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
🔇 Additional comments (12)
.changeset/witty-actors-dress.md (1)
1-5: Changeset structure looks goodThe changeset correctly documents the feature addition with the appropriate versioning level (patch).
target.json (1)
6-6: Significant reduction in initial WebAssembly memoryThe initial memory allocation has been reduced from 50MB to 10MB. While this will improve load time and resource usage in browser environments, ensure this has been thoroughly tested with memory-intensive operations to prevent potential out-of-memory issues.
Has this memory reduction been tested with the full range of operations the library supports? Memory-intensive operations that worked previously might be affected by this change.
package.json (1)
36-39: Browser-specific export entry properly configuredThe addition of a dedicated browser entry point is well-structured and follows package.json best practices with properly defined types and implementation paths.
test/fixtures.spec.ts (1)
1-1: Improved use of promise-based filesystem APIThe code now directly imports and uses the promise-based fs API, which is cleaner and follows current Node.js best practices for asynchronous file operations.
Also applies to: 9-10
README.md (3)
23-24: Well-structured addition of node/browser subsections!The addition of separate node and browser subsections improves the documentation structure and makes it clearer how to use the library in different environments.
58-73: Good documentation of browser loading options!The documentation clearly illustrates two common methods for loading WASM in a browser environment:
- Using
fetchwith a CDN URL- Using Vite's
wasm?initfeatureThis gives developers flexibility in how they integrate the library.
95-122: Performance improvements are impressive!The benchmark results show a significant performance improvement with average iteration time decreasing from ~37ms to ~18ms, making the library over 4x faster than the alternative. This is a notable achievement.
benchmark/benchmark.txt (1)
1-28: Great performance improvement!The benchmark shows approximately 2x speedup in execution time compared to previous versions, with
sh-syntaxnow being 4.34x faster thanmvdan-sh. These performance improvements are substantial and should be highlighted in release notes.🧰 Tools
🪛 LanguageTool
[grammar] ~15-~15: Make sure that the singular noun after the number ‘312.45’ is correct.
Context: ... (291.31 kb … 312.45 kb) 292.09 kb ████▇▇▇▇▁▁▁▁▇▁██▇████ ...(CD_POINT_CD_NN)
test/processor.spec.ts (2)
12-24: Good workaround for Vitest issue with WASM importsYou've provided a clear implementation for loading WASM in tests with detailed comments about why this approach is needed (Vitest issue #6723). The implementation correctly reads and instantiates the WASM file.
27-37: Clean implementation of parse and print functionsThese functions mirror the API provided in the main package, ensuring consistent testing of the actual functionality that users will access.
src/processor.ts (2)
27-34: Well-defined types for WebAssembly loading optionsThe newly added types
GetWebAssemblySourceandGetWebAssemblyInstanceclearly define the contract for providing WebAssembly content, supporting both synchronous and asynchronous patterns.
45-46: Improved encoder/decoder initializationUsing the nullish coalescing assignment operator (
??=) ensures encoders/decoders are only created once if they're undefined, which is more efficient than recreating them on each call.
|
@coderabbitai No, but thanks, your suggestion is not type safe for users, they could miss the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@JounQin You're absolutely right. Using function arity for type differentiation isn't type-safe since TypeScript doesn't enforce parameter counts at runtime, and users could easily create implementations with optional parameters that would bypass the arity check. This could lead to unexpected behavior where the wrong implementation path is taken. Thank you for the clarification - I agree with your approach to prioritize type safety for users. 🏁 Script executed: Length of output: 8122 @JounQin I see your point now after examining the implementation. You're absolutely right that relying on function arity is type-unsafe. If users implement a WebAssembly instance provider with optional parameters: const myProvider = (imports?: WebAssembly.Imports) => {...}Its Thank you for clarifying - your approach to prioritize type safety is the right decision here.✏️ Learnings added
|
Summary by CodeRabbit
New Features
Documentation
Chores
Tests