refactor(js)!: run WASM calls on the caller's thread instead of a Worker - #510
Conversation
Remove the internal Worker (browser) / worker_threads (Node) indirection. wasmClient now calls into the Emscripten module directly via ccall + Asyncify, with no message-passing layer in between. BREAKING CHANGE: img2num no longer offloads WASM execution to a background thread automatically. Heavy operations (gaussianBlur, bilateralFilter, kmeans, imageToSvg) now run on whichever thread calls them — this can block the browser main thread/UI or the Node event loop during processing. Consumers who need non-blocking behavior must now wrap calls in their own Worker or worker_thread.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe WASM client now runs in-thread with centralized lifecycle management and typed heap buffers instead of workers. The package exposes cleanup, adds an HTML example app, updates consumers, and revises Vite, workspace, documentation, and development-container configuration. ChangesWASM execution and example integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
build-system
important
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/js/src/wasmClient.js`:
- Around line 141-142: Preserve the original throwable in the catch block of the
wasm client initialization method instead of interpolating only error.message;
create the contextual error with the original value attached as its cause, while
still handling non-Error throwables safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 333b0303-00d1-45e1-aaeb-8caf4a680801
📒 Files selected for processing (5)
packages/js/src/target/browser/worker.jspackages/js/src/target/node/worker.jspackages/js/src/wasmClient.jspackages/js/src/workers/wasmWorker.jspackages/js/vite.config.js
💤 Files with no reviewable changes (3)
- packages/js/src/target/browser/worker.js
- packages/js/src/target/node/worker.js
- packages/js/src/workers/wasmWorker.js
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CodeRabbit / Review
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Lint & Validate Code
- GitHub Check: Analyze (c-cpp)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
packages/js/src/wasmClient.jspackages/js/vite.config.js
**
⚙️ CodeRabbit configuration file
**: # Contributing to Img2NumWant to contribute to Img2Num? There are a few things you need to know.
We wrote a contribution guide to help you get started.
A few important points:
- Add tests with your PR — new features and bug fixes must include tests where appropriate. PRs without tests are unlikely to be approved.
- Follow the repository's coding style rules.
- Use the issue and PR templates when filing issues or submitting code. Your PR will be rejected if you don't.
If you're unsure what to change, open a discussion and someone will assist you.
Questions?
If you have questions or need help:
- Open a discussion
- Create an issue
- Check existing PRs for ideas
Thank you for improving Img2Num! 🎨🚀
**: BasedOnStyle: LLVM
Standard: c++20--- Basic formatting ---
IndentWidth: 4
ColumnLimit: 100
TabWidth: 4--- Braces ---
Cpp11BracedListStyle: true
SpaceBeforeCpp11BracedList: true
BreakBeforeBraces: Attach--- Braced initializers ---
Cpp11BracedListStyle: true
--- Constructor initializer lists ---
PackConstructorInitializers: Never
BreakConstructorInitializers: BeforeComma
ConstructorInitializerIndentWidth: 4--- Alignment ---
AlignAfterOpenBracket: BlockIndent
--- Pointers ---
PointerAlignment: Left
--- Includes ---
IncludeBlocks: Regroup
SortIncludes: CaseInsensitive--- Extern "C" cleanliness ---
IndentExternBlock: NoIndent
--- Lambdas ---
AllowShortLambdasOnASingleLine: Inline
--- Functions ---
AllowShortFunctionsOnASingleLine: None
**: root = true-------------------------
Global defaults
-------------------------
[*]
charset = utf-8
end_of_line = lf
indent_style = space
indent_size = 2
trim_trailing_whitespace = true
insert_final_newline = true
max_line_length = 120-------------...
Files:
packages/js/src/wasmClient.jspackages/js/vite.config.js
🔇 Additional comments (5)
packages/js/vite.config.js (2)
12-13: LGTM!Also applies to: 47-48
33-35: 🗄️ Data Integrity & IntegrationKeep Node built-ins external in the Node bundle. The shorter external list is fine only if this build stays on Vite’s Node SSR path; otherwise the WASM glue can pull in
fs/path/other built-ins that need explicit externalization.packages/js/src/wasmClient.js (3)
67-86: 🩺 Stability & AvailabilityCheck that failed initialization can be retried. If
createImg2NumModule()orinitWebGPU()rejects, clearreadyPromiseso a laterinitWasmWorker()call can retry.
143-150: 🩺 Stability & AvailabilityWebGPU teardown may be too aggressive
destroyWebGPU()runs in thefinallyofcallWasm, butinitWebGPU()is only performed once. If WebGPU is meant to live across calls, this will leave later invocations using a torn-down GPU context.
158-164: 🩺 Stability & AvailabilityConsider awaiting WebGPU teardown.
terminateWasmWorkerkicks offdestroyWebGPU()without awaiting it, then clears the module state immediately. If callers can re-init right away, cleanup may overlap with the next worker start.
|
Hi @Krasner and @Prachi-Gupta2808. Please will you let me know your thoughts here. I do still need to refactor the code to update variable names and stuff like that so it makes sense and is easier to maintain, though. I've basically removed the worker-related code because it previously only worked with Vite and workers aren't available everywhere. Instead of making it compatible with other bundlers, I just removed it because doing so will allow the library to be able to run in other environments that don't support workers. Our C, C++, and Python library don't do what the library currently does in v0.2.x. The JavaScript library is the only one that attempts to prevent the main thread from being blocked and I think that's a bad design choice because it locks users into that way of optimizing it. As a result, I removed the workers entirely for these reasons:
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
so it directly calls into |
|
check build error: |
coderabbitai
Bot
removed
tooling
important
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/js/src/wasmClient.js (1)
143-150: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSerialize teardown with in-flight calls and await destruction.
terminateWasmWorker()can setwasmModuletoundefinedwhilecallWasm()is suspended inccallAsync; itsfinallyblock then dereferences the cleared module. Additionally, bothdestroyWebGPU()calls are fire-and-forget, so teardown races with later initialization and rejected promises become unhandled. Track/await active calls before resetting state and centralize awaited teardown.Also applies to: 158-164
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/js/src/wasmClient.js` around lines 143 - 150, Update terminateWasmWorker() and callWasm() to serialize teardown with in-flight ccallAsync operations: track active calls, await their completion before clearing wasmModule, and ensure callWasm() cleanup does not dereference cleared state. Centralize the node destroyWebGPU() logic in an awaited teardown path, replacing both fire-and-forget calls and propagating any rejection through the existing lifecycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/js/src/wasmClient.js`:
- Around line 143-150: Update terminateWasmWorker() and callWasm() to serialize
teardown with in-flight ccallAsync operations: track active calls, await their
completion before clearing wasmModule, and ensure callWasm() cleanup does not
dereference cleared state. Centralize the node destroyWebGPU() logic in an
awaited teardown path, replacing both fire-and-forget calls and propagating any
rejection through the existing lifecycle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aa5eeb76-f71d-49d4-92e3-8eac579ef818
📒 Files selected for processing (1)
packages/js/src/wasmClient.js
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Lint & Validate Code
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
packages/js/src/wasmClient.js
🔇 Additional comments (2)
packages/js/src/wasmClient.js (2)
3-61: LGTM!Also applies to: 67-86, 88-92, 106-132, 139-140
133-138: 🎯 Functional CorrectnessNo action needed in this helper
callWasmis internal, and the exported wrappers only usereturnType: "string", so thenumber/typed-array cases here aren’t reachable from the JS package API.> Likely an incorrect or invalid review comment.
github-actions
Bot
added
example-app
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
coderabbitai
Bot
removed
the
important
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eslint.config.js`:
- Line 8: Restore explicit ESLint ignore entries for example-apps/react-js/dist
and example-apps/react-js/node_modules alongside the existing example-apps
ignore configuration, ensuring generated output and installed dependencies
remain excluded from lint traversal.
In `@example-apps/html-js/index.html`:
- Line 41: Remove the invalid inline padding declaration from the themeToggle
button. Keep the button’s existing id, classes, and type unchanged, relying on
the existing stylesheet for its spacing.
- Around line 42-43: Replace the self-closing div in the Docusaurus style
workaround with a valid paired div element by adding an explicit closing tag,
preserving the existing display:none behavior without nesting subsequent
document elements.
- Line 64: Add an element with id="uploadHelp" near the control using
aria-describedby, and place the upload guidance text inside it so the existing
accessibility reference resolves correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59b402ab-6e0d-4503-a72f-fd846ed61c13
📒 Files selected for processing (6)
eslint.config.jsexample-apps/html-js/index.htmlexample-apps/react-js/src/components/GlassCard.jsxpackages/js/README.mdpackages/js/src/wasmClient.jspackages/js/src/wasmModule.js
💤 Files with no reviewable changes (1)
- example-apps/react-js/src/components/GlassCard.jsx
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Build C/C++ / Build C & C++
- GitHub Check: Build C/C++ / Build Python
- GitHub Check: Build C/C++ / Build WASM (bindings/js)
- GitHub Check: Lint & Validate Code
🧰 Additional context used
📓 Path-based instructions (4)
**/*.md
📄 CodeRabbit inference engine (.editorconfig)
**/*.md: Do not trim trailing whitespace in Markdown files
Use 2-space indentation for Markdown files
Do not enforce maximum line length for Markdown files
Files:
packages/js/README.md
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{js,ts,jsx,tsx}: Use 2-space indentation for JavaScript and TypeScript files
Maintain 200 character maximum line length for JavaScript/TypeScript files
Files:
eslint.config.jspackages/js/src/wasmModule.jspackages/js/src/wasmClient.js
**/*.{html,htm}
📄 CodeRabbit inference engine (.editorconfig)
**/*.{html,htm}: Use 2-space indentation for HTML files
Maintain 200 character maximum line length for HTML files
Files:
example-apps/html-js/index.html
example-apps/**
⚙️ CodeRabbit configuration file
example-apps/**: - Example applications. These are for demonstration; keep them minimal and ensure
they correctly reflect the public API. Flag any use of internal/private APIs.
- IMPORTANT: These example apps must be a good reflection of how to use Img2Num's
library, which means that they need good comments and must have clean code. This
is meant to be external and is designed for others to use to get started.
Files:
example-apps/html-js/index.html
🧠 Learnings (1)
📚 Learning: 2026-02-22T15:20:15.304Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 263
File: docs/docs/internal/core/api-reference.md:13-13
Timestamp: 2026-02-22T15:20:15.304Z
Learning: In Docusaurus projects, Markdown files (*.md, *.mdx) may contain anchor tags with onClick={(e) => { e.preventDefault(); window.location.href = '...'; }} to force a full page navigation, bypassing React SPA routing for static/external paths. Treat this as intentional and do not flag it as redundant in docs contexts. Apply this guidance broadly to Markdown docs across the repository (not just the single file) and focus reviews on more impactful patterns such as accessibility, SEO, and navigation consistency.
Applied to files:
packages/js/README.md
🔇 Additional comments (4)
packages/js/src/wasmModule.js (1)
43-52: LGTM!packages/js/src/wasmClient.js (1)
9-9: LGTM!packages/js/README.md (1)
167-174: LGTM!example-apps/html-js/index.html (1)
2-2: LGTM!Also applies to: 22-24, 111-113, 179-185
|
@Ryan-Millard can you merge this? Then I can work on the comparison demo page |
Will do. I just want to check that everything is okay one last time. This will be released as part of v0.3 in the JS package. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai suggest a commit message and description. I was considering the below for release-please compatibility with multiple CHANGELOG.md items because this covers a lot: refactor(JavaScript Workers)!: run WASM calls on the caller's thread instead of a Worker (#510)
Remove the internal Worker (browser) / worker_threads (Node) indirection.
wasmClient now calls into the Emscripten module directly via ccall +
Asyncify, with no message-passing layer in between.
BREAKING CHANGE: img2num no longer offloads WASM execution to a background
thread automatically. Heavy operations (gaussianBlur, bilateralFilter,
kmeans, imageToSvg) now run on whichever thread calls them.
This can block the calling thread (browser main thread/UI or the Node event loop during
processing. Consumers who need non-blocking behavior must now wrap
calls in their own Worker or worker_thread.
feat(example app): add html-js example app to demo basic library usage
docs(website): update documentation based on refactor in #510
docs(CSS): fix table of contents styling and layout
docs: update JSDoc comments and break up website docs
---------
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
|
Tip For best results, initiate chat on the files or code changes.
refactor(js)!: run WASM calls on the caller thread (`#510`)
Remove the internal browser Worker and Node worker_threads indirection.
wasmClient now invokes the Emscripten module directly through ccallAsync
and Asyncify, eliminating the message-passing layer and Vite-specific
worker setup.
Add the html-js example application and update the JavaScript package
documentation to show explicit WASM-module cleanup.
BREAKING CHANGE: img2num no longer automatically runs WASM operations in
a background thread. Heavy operations such as gaussianBlur,
bilateralFilter, kmeans, and imageToSvg execute on the thread that calls
them, which can block the browser main thread or Node.js event loop.
Consumers requiring non-blocking execution must invoke img2num from their
own Worker or worker_thread.A few small recommendations:
|
4 participants
Changes & Reason
Changes
Remove workers entirely to have better cross-environment support. Previously, the JavaScript library only worked in Vite-bundled environments. Additionally, workers also led to problems as it would require additional setup from users to get started.
packages/jsrefactor around the loss of workersexample-apps/html-jsexample-apps/html-jsin withdocslikeexample-apps/react-jsReason
This reduces the burden on us to maintain a complex worker setup whilst giving consumers the freedom to choose how they want their applications to work. The main reason for the removal of the worker is to shift the responsibility onto the consumer (who should be more than capable) and allow them to freely choose whether they even need to manage the blocking nature of this library.
None of our other bindings handled the blocking nature of the library, so the JavaScript library should follow suit.
Related Issues
Fixes: #470
Test these changes
Unzip the files (example HTML and built library):
example-img2num.zip
Run this from the same folder:
Additional Resources
The video below shows how
example-apps/html-jsworks - similar toexample-apps/react-js.Important
Watch my cursor after uploading the image. Before it flew to the top-left corner of the screen,
I right-clicked.
Once the image processing finishes, the dropdown shows. This delay is caused by the removal
of the worker setup in
packages/js- hence the breaking change.The library now runs synchronously and it is up to the consumer of the library to determine
whether they can tolerate it or not. They're welcome to use workers, however we have opted
to remove them since workers are incompatible with certain JavaScript environments.
Video.Project.2.mp4
TODO:
packages-js-v0.3)