-
Notifications
You must be signed in to change notification settings - Fork 54
fix: torii-wasm node import #455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis change introduces a new ES module wrapper for the Changes
Sequence Diagram(s)sequenceDiagram
participant NodeWorker
participant node.mjs (ESM)
participant dojo_c.js (CJS)
NodeWorker->>node.mjs (ESM): import { ... } from 'torii-wasm/node'
node.mjs (ESM)->>dojo_c.js (CJS): createRequire + require('dojo_c.js')
node.mjs (ESM)-->>NodeWorker: re-export all named and default exports
Possibly related PRs
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/shaky-wasps-invent.md (2)
1-13
: Align changeset scope with summary
The changeset bumps patch versions for multiple packages (core, SDK, react, etc.) but the summary only mentionstorii-wasm
. Consider updating the summary to reflect the multi-package patch release or restrict the changeset to onlytorii-wasm
if that was the intent.
15-15
: Enhance changeset description
Add more context to the summary line, for example: “add Node.js ESM wrapper via node.mjs for torii-wasm import fix” to improve clarity in release notes.packages/torii-wasm/node.mjs (2)
1-3
: Use explicitnode:
protocol for built-ins
Node.js recommends using thenode:
specifier for built-in modules.-import { createRequire } from 'module'; +import { createRequire } from 'node:module';
5-29
: Consider simplifying re-exports
Manually enumerating every export fromdojo_c
can become brittle as the API evolves. You might replace the manual list with a generic re-export approach:import { createRequire } from 'node:module'; const require = createRequire(import.meta.url); const dojo_c = require('./pkg/node/dojo_c.js'); export * from './pkg/node/dojo_c.js'; export { default } from './pkg/node/dojo_c.js';This automatically surfaces new exports without needing to update this file each time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.changeset/shaky-wasps-invent.md
(1 hunks)packages/torii-wasm/node.mjs
(1 hunks)packages/torii-wasm/package.json
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/torii-wasm/node.mjs
[error] 1-1: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.
(lint/style/useNodejsImportProtocol)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (2)
packages/torii-wasm/package.json (2)
25-26
: ```shell
#!/bin/bashRe-verify that node.mjs is included by running npm pack in the package directory
cd packages/torii-wasm
npm pack --dry-run | grep node.mjs--- `34-38`: **Validate dual export for Node subpath** The `./node` export now points `"import": "./node.mjs"` and `"require": "./pkg/node/dojo_c.js"`. Confirm both ESM (`import '@dojoengine/torii-wasm/node'`) and CJS (`require('@dojoengine/torii-wasm/node')`) resolutions behave as expected, especially in worker contexts. ```shell #!/bin/bash # Check ESM import node -e "import('@dojoengine/torii-wasm/node').then(m => console.log('ESM load OK')).catch(err => console.error(err))" # Check CJS require node -e "console.log('CJS load OK', require('@dojoengine/torii-wasm/node'))"
Closes #
Introduced changes
Checklist
Summary by CodeRabbit
Bug Fixes
Chores