fix: always use wasm32-wasi target for webcontainer
#23
Conversation
|
WalkthroughThis update introduces specialized handling for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainModule as src/index.ts
participant Helpers as src/helpers.ts
participant NPM
participant FS as File System
User->>MainModule: Initiate installUsingNPM(target)
MainModule->>Helpers: getNapiNativeTargets()
Helpers-->>MainModule: Return ['wasm32-wasi'] if webcontainer, else targets
alt Target is wasm32-wasi
MainModule->>NPM: npm install with --cpu=wasm32 --force
MainModule->>FS: Move all temp node_modules to parent node_modules
MainModule->>FS: Remove temp package.json
MainModule->>FS: Use .wasm extension for binary
else Other targets
MainModule->>NPM: npm install (standard flags)
MainModule->>FS: Move specific package dir to parent node_modules
MainModule->>FS: Use .node extension for binary
end
Possibly related PRs
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
src/helpers.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs src/index.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs ✨ 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:
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.
Pull Request Overview
This PR fixes the target selection for the webcontainer by ensuring that the wasm32-wasi target is always used, and updates related npm installation commands and subpath resolution.
- Update npm install flags and file handling to support wasm32-wasi
- Adjust subpath construction to return the proper file extension based on the target
- Add a webcontainer check in getNapiNativeTargets to force the WASI target
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/index.ts | Updates to npm install command, package.json handling, and subpath resolution for wasm32-wasi support |
| src/helpers.ts | Introduces a webcontainer check to enforce WASI target usage |
Files not reviewed (1)
- .size-limit.json: Language not supported
Comments suppressed due to low confidence (2)
src/index.ts:298
- Consider adding tests to verify the correct subpath resolution for both wasm and node targets, ensuring that the filename extension adjusts appropriately based on the target.
const subpath = `${binaryPrefix}${target}.${target === WASM32_WASI ? 'wasm' : 'node'}`
src/helpers.ts:258
- Ensure adequate test coverage is in place for scenarios where process.versions.webcontainer is defined so that the function correctly returns the [WASI_TARGET] array.
if (process.versions.webcontainer) {
commit: |
|
Deploy preview for napi-postinstall ready! ✅ Preview Built with commit c893d10. |
size-limit report 📦
|
📊 Package size report 7%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
==========================================
- Coverage 39.02% 36.34% -2.68%
==========================================
Files 6 6
Lines 492 531 +39
Branches 53 55 +2
==========================================
+ Hits 192 193 +1
- Misses 300 338 +38 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/helpers.ts (1)
258-260: Add unit tests for the newwebcontainerfast-path
getNapiNativeTargets()now returns early whenprocess.versions.webcontaineris truthy, but the added branch is not covered by tests (see Codecov warning). A trivial unit test can stubprocess.versions.webcontainerand assert that only['wasm32-wasi']is returned. Verifying this guards against future refactors that might inadvertently break the WebContainer experience.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 259-260: src/helpers.ts#L259-L260
Added lines #L259 - L260 were not covered by testssrc/index.ts (2)
84-88: Nit: local boolean name gives stronger intent
const isWasm32Wasi = target === WASM32_WASIis clear, but because you branch on this flag ~40 lines later it’s easy to forget the semantic. A slightly more specific name (targetIsWasi) improves readability for future readers skimming the file.-const isWasm32Wasi = target === WASM32_WASI +const targetIsWasi32 = target === WASM32_WASI🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 86-86: src/index.ts#L86
Added line #L86 was not covered by tests
122-157: Directory shuffle can be replaced with one atomic moveThe loop iterates over every package and attempts
fs.renameSync, swallowingEEXIST. Two drawbacks:• Performance:
renameis invoked O(number of packages).
• Partial state: if the process dies midway the install may be left half-moved.Consider a simpler and atomic alternative:
-if (isWasm32Wasi) { - const newNodeModulesDir = path.resolve(installDir, '../node_modules') - fs.rmSync(newNodeModulesDir, { recursive: true, force: true }) - fs.renameSync(nodeModulesDir, newNodeModulesDir) -} else { +if (!isWasm32Wasi) { // existing single-package move
fs.renameSyncon the wholenode_modulestree is fast (it just patches directory entries) and either succeeds entirely or leaves the original tree untouched.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 122-126: src/index.ts#L122-L126
Added lines #L122 - L126 were not covered by tests
[warning] 129-140: src/index.ts#L129-L140
Added lines #L129 - L140 were not covered by tests
[warning] 143-151: src/index.ts#L143-L151
Added lines #L143 - L151 were not covered by tests
[warning] 154-157: src/index.ts#L154-L157
Added lines #L154 - L157 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/fair-pillows-divide.md(1 hunks).size-limit.json(1 hunks)src/helpers.ts(1 hunks)src/index.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/index.ts (1)
src/constants.ts (3)
WASM32_WASI(15-15)PACKAGE_JSON(19-19)WASM32(13-13)
🪛 GitHub Check: codecov/patch
src/helpers.ts
[warning] 259-260: src/helpers.ts#L259-L260
Added lines #L259 - L260 were not covered by tests
src/index.ts
[warning] 86-86: src/index.ts#L86
Added line #L86 was not covered by tests
[warning] 99-99: src/index.ts#L99
Added line #L99 was not covered by tests
[warning] 101-101: src/index.ts#L101
Added line #L101 was not covered by tests
[warning] 111-111: src/index.ts#L111
Added line #L111 was not covered by tests
[warning] 117-119: src/index.ts#L117-L119
Added lines #L117 - L119 were not covered by tests
[warning] 122-126: src/index.ts#L122-L126
Added lines #L122 - L126 were not covered by tests
[warning] 129-140: src/index.ts#L129-L140
Added lines #L129 - L140 were not covered by tests
[warning] 143-151: src/index.ts#L143-L151
Added lines #L143 - L151 were not covered by tests
[warning] 154-157: src/index.ts#L154-L157
Added lines #L154 - L157 were not covered by tests
[warning] 167-177: src/index.ts#L167-L177
Added lines #L167 - L177 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
🔇 Additional comments (3)
src/index.ts (1)
297-299: Confirm.wasmresolution works withrequire.resolve
require.resolve()is used on*.wasm. Node ≥ 16 resolves.wasmautomatically only when run with--experimental-wasm-modules; otherwise it must be imported viafs.readFileSync/ WebAssembly.instantiate. Ensure:
- The runtime actually loads the path via
fs/URLrather thanrequire(), or- The project sets
"type": "module"and the proper loader flag, or- A custom loader/hook is in place.
If none of the above is true, this line will resolve the path but later
require()will throw “Unexpected file extension”..size-limit.json (1)
4-4: Size-limit bump looks fineThe increase from 2.5 kB → 2.6 kB matches the extra WASI code paths; nothing else to flag.
.changeset/fair-pillows-divide.md (1)
1-6: Changelog entry LGTMClear, concise description of the behavioural change.
Summary by CodeRabbit
Bug Fixes
Chores