fix: support install wasm32-wasi correctly with --cpu=wasm32
#18
Conversation
🦋 Changeset detectedLatest commit: 3d9a839 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 update introduces new constants and type definitions to support the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant checkAndPreparePackage
participant installUsingNPM
participant NPM
User->>checkAndPreparePackage: Initiate package preparation
checkAndPreparePackage->>installUsingNPM: Call with (hostPkg, pkg, version, target, subpath, nodePath)
alt target is WASM32_WASI
installUsingNPM->>NPM: npm install ... --cpu=wasm32
else Other target
installUsingNPM->>NPM: npm install ...
end
NPM-->>installUsingNPM: Installation result
installUsingNPM-->>checkAndPreparePackage: Return status
checkAndPreparePackage-->>User: Completion/result
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
lib/constants.d.tsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs lib/constants.jsOops! Something went wrong! :( ESLint: 9.25.1 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.mjs lib/index.jsOops! 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 installation of the wasm32-wasi target by updating type definitions, target parsing logic, and installation commands to correctly handle the --cpu flag.
- Updated type definitions in src/types.ts for Platform, NodeJSArch, and Target.
- Revised triple parsing logic in both src/target.ts and lib/target.js using new constants.
- Modified installation commands in src/index.ts and lib/index.js to support the wasm32-wasi target.
Reviewed Changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Introduced new types to include 'wasi' and 'wasm'/platform values. |
| src/target.ts | Updated triple parsing to use constants and removed redundant type declarations. |
| src/index.ts | Adjusted installUsingNPM call and package version comparisons. |
| src/constants.ts | Added constants for WASM32, WASI, WASM32_WASI, and EABI. |
| lib/target.js | Mirrored triple parsing updates from src/target.ts. |
| lib/index.js | Updated npm installation command to conditionally set the --cpu flag. |
| lib/constants.js | Updated constants consistent with the src version. |
Files not reviewed (2)
- .npmrc: Language not supported
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/target.ts:52
- [nitpick] Consider using EABI.length instead of the hardcoded value 4 when slicing rawTriple to enhance clarity and maintainability.
const triple = rawTriple.endsWith(EABI) ? `${rawTriple.slice(0, -4)}-${EABI}` : rawTriple
lib/target.js:32
- [nitpick] Consider using constants_js_1.EABI.length in place of the hardcoded value 4 when slicing rawTriple, to improve code clarity and reduce potential errors.
const triple = rawTriple.endsWith(constants_js_1.EABI) ? `${rawTriple.slice(0, -4)}-${constants_js_1.EABI}` : rawTriple;
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📊 Package size report 2%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
commit: |
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
===========================================
+ Coverage 39.95% 55.79% +15.83%
===========================================
Files 6 6
Lines 458 466 +8
Branches 53 67 +14
===========================================
+ Hits 183 260 +77
+ Misses 275 206 -69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Deploy preview for napi-postinstall ready! ✅ Preview Built with commit 3d9a839. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.changeset/neat-bats-scream.md (1)
1-6: Reference related issue in changeset. Consider adding a link tounrs/unrs-resolver#95in the changeset body to provide context for this patch-level fix. For example:fix: support install `wasm32-wasi` correctly with `--cpu=wasm32` Closes unrs/unrs-resolver#95package.json (1)
10-10: Consider documenting the reason for package manager downgrade.Downgrading from npm@11.3.0 to npm@10.9.2 might have been done for specific compatibility reasons. Consider adding a comment or updating the README to explain why this change was necessary.
lib/index.js (1)
130-130: Correctly updated function calls with new parameters.The calls to
installUsingNPManddownloadDirectlyFromNPMhave been properly updated to use the correct version parameter and pass the target parameter where needed.I'd recommend adding a small unit test to verify the
--cpu=wasm32flag is correctly added for wasm32-wasi targets.Also applies to: 136-136
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
lib/constants.js.mapis excluded by!**/*.maplib/index.js.mapis excluded by!**/*.maplib/target.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.changeset/neat-bats-scream.md(1 hunks).npmrc(1 hunks)lib/constants.d.ts(1 hunks)lib/constants.js(1 hunks)lib/index.d.ts(0 hunks)lib/index.js(3 hunks)lib/target.d.ts(1 hunks)lib/target.js(2 hunks)lib/types.d.ts(1 hunks)package.json(3 hunks)src/constants.ts(1 hunks)src/index.ts(6 hunks)src/target.ts(2 hunks)src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- lib/index.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
lib/index.js (4)
lib/helpers.js (7)
pkg(65-65)version(62-62)path(12-12)fs(11-11)node_child_process_1(10-10)constants_js_1(13-13)napi(55-55)lib/constants.js (1)
path(5-5)src/constants.ts (1)
require(7-9)lib/target.js (1)
constants_js_1(4-4)
src/target.ts (1)
src/constants.ts (4)
WASM32_WASI(15-15)WASM32(13-13)WASI(14-14)EABI(17-17)
lib/constants.d.ts (1)
src/constants.ts (4)
WASM32(13-13)WASI(14-14)WASM32_WASI(15-15)EABI(17-17)
lib/types.d.ts (1)
src/types.ts (3)
Platform(35-35)NodeJSArch(37-37)Target(39-45)
🪛 GitHub Check: codecov/patch
src/index.ts
[warning] 197-197: src/index.ts#L197
Added line #L197 was not covered by tests
[warning] 254-254: src/index.ts#L254
Added line #L254 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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
- 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 (21)
src/constants.ts (1)
13-17: Centralize target strings with new constants. IntroducingWASM32,WASI,WASM32_WASI, andEABIimproves maintainability and avoids magic strings across modules.
Please verify that all hard-coded references to these values insrc/target.tsandsrc/index.tshave been replaced with the new constants.lib/target.d.ts (1)
1-2: Validate import path for type declarations. ImportingTargetfrom./types.jsin a.d.tsfile may require specific module resolution settings (e.g.,"moduleResolution": "node16"), or alternatively dropping the.jsextension for better compatibility. Please verify your TypeScript configuration supports this import pattern.lib/constants.d.ts (1)
4-7: Add new WASM and ABI constants in declarations. The new constant declarations forWASM32,WASI,WASM32_WASI, andEABIcorrectly mirror the runtime definitions, keeping the type declarations in sync with implementation.lib/constants.js (1)
9-12: Well-structured addition of WebAssembly-related constants.Adding these constants is a good practice for improving maintainability and consistency across the codebase. The constants are clearly named and I like how
WASM32_WASIis constructed using template literals from the other constants.package.json (1)
59-94: Good maintenance of dependencies.The dependency updates look appropriate and keeping dependencies up-to-date is a good security practice.
lib/target.js (2)
4-4: Excellent refactoring to use constants.Replacing hardcoded string literals with constants improves code maintainability and consistency. The conditional checks are now more readable and less prone to typos.
Also applies to: 20-28
31-33: Good use of constants for EABI suffix check.Refactoring the EABI suffix check to use the constant improves consistency and reduces the chance of errors in future modifications.
lib/index.js (3)
57-57: Well-designed function signature update.Adding the
targetparameter to theinstallUsingNPMfunction enhances the API to support WebAssembly architectures.
64-64: Good conditional flag addition for wasm32 CPU target.The conditional addition of
--cpu=wasm32forwasm32-wasitargets is the core fix for the issue in the PR description. This will correctly handle the installation for WebAssembly targets.
100-103: Good variable renaming to avoid shadowing.Renaming the destructured
versiontopkgVersionand usingversion = pkgVersionas a fallback prevents variable shadowing and makes the code clearer. The version consistency check is appropriately updated to use the new variable names.lib/types.d.ts (1)
29-37: Good addition of WebAssembly and WASI type support.The new type definitions effectively extend NodeJS platform and architecture types to support WebAssembly environments. The
Targetinterface properly encapsulates all necessary properties for target platform identification.src/types.ts (1)
35-45: Well-structured type definitions with proper spacing.These type definitions mirror those in lib/types.d.ts, maintaining consistency across the TypeScript source and declaration files. Good organization with appropriate spacing between type declarations for readability.
src/target.ts (3)
3-4: Good refactoring to use centralized types and constants.Importing types from a dedicated module and constants instead of using string literals improves maintainability.
40-49: Improved code with constants replacing magic strings.Using the imported constants (WASM32_WASI, WASM32, WASI) instead of hardcoded string literals makes the code more maintainable and reduces the risk of typos or inconsistencies.
52-53: Good use of constant for EABI check.Replacing the hardcoded 'eabi' string with the EABI constant maintains consistency with the other constant usage in this file.
src/index.ts (6)
9-9: Good import of required constants.Added import for the WebAssembly-related constants that will be used later in the conditional logic.
79-79: Critical parameter addition for target-specific installation.Adding the
targetparameter toinstallUsingNPMis essential for implementing the WebAssembly-specific flag handling.
103-105: Key implementation for wasm32-wasi support.This conditional addition of the
--cpu=wasm32flag when the target is WASM32_WASI is the core change needed to fix the installation support for the wasm32-wasi target, which aligns with the PR objective.
188-188: Good variable renaming to avoid shadowing.Renaming
versiontopkgVersionin destructuring avoids variable shadowing in the subsequent code.
190-193: Proper variable naming with good default.Setting
version = pkgVersionas a default value ensures backward compatibility and provides a sensible fallback.
243-243: Properly passing the target parameter.Updated function call to include the new target parameter, ensuring it's available for the conditional CPU flag logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/index.ts (1)
75-83: 🛠️ Refactor suggestionExplicitly type the new
targetparameter
installUsingNPM()now accepts atargetargument but it’s typed generically asstring.
Since you already have aTarget/NodeJSArchunion insrc/types.ts, using that type will:
- give callers autocomplete / compile-time safety (only recognised targets compile),
- prevent accidental typos (e.g.
"wasm32_wasi"vs"wasm32-wasi"),- reduce the risk of injecting unexpected shell flags later on.
-function installUsingNPM( - hostPkg: string, - pkg: string, - version: string, - target: string, +function installUsingNPM( + hostPkg: string, + pkg: string, + version: string, + target: Target, // <-- or `NodeJSArch` depending on which is correct
🧹 Nitpick comments (2)
src/index.ts (2)
103-105: Shell-injection surface – quote interpolated values
pkgandversionultimately come frompackage.json; still, quoting is a good defensive habit and prevents accidental breaks when a version contains shell-metacharacters (e.g. pre-release tags like1.0.0-beta.1).- } ${pkg}@${version}`, + } "${pkg}@${version}"`,This also keeps behaviour consistent across Windows where caret (
^) in version strings can be interpreted bycmd.exe.
188-199: Add tests for the new version-consistency branchStatic analysis flags lines 195-197 as uncovered. A simple happy-path test exercising the mismatch branch will lock in behaviour:
- Create a dummy
package.jsonwithversion: "1.0.0".- Stub
getNapiInfoFromPackageJsonto return{ napi: { packageName: 'dummy' }, version: '2.0.0' }.- Expect
checkAndPreparePackage(pkgJson, true)to throw the exact error.Would you like assistance scaffolding that unit test?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 197-197: src/index.ts#L197
Added line #L197 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
lib/constants.js.mapis excluded by!**/*.maplib/index.js.mapis excluded by!**/*.maplib/target.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.changeset/neat-bats-scream.md(1 hunks).npmrc(1 hunks)lib/constants.d.ts(1 hunks)lib/constants.js(1 hunks)lib/index.d.ts(0 hunks)lib/index.js(3 hunks)lib/target.d.ts(1 hunks)lib/target.js(2 hunks)lib/types.d.ts(1 hunks)package.json(3 hunks)src/constants.ts(1 hunks)src/index.ts(6 hunks)src/target.ts(2 hunks)src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- lib/index.d.ts
✅ Files skipped from review due to trivial changes (1)
- src/target.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- .npmrc
- lib/target.d.ts
- .changeset/neat-bats-scream.md
- src/constants.ts
- src/types.ts
- lib/constants.js
- lib/constants.d.ts
- lib/index.js
- lib/types.d.ts
- package.json
- lib/target.js
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/index.ts
[warning] 197-197: src/index.ts#L197
Added line #L197 was not covered by tests
[warning] 254-254: src/index.ts#L254
Added line #L254 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on ubuntu-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
- GitHub Check: Lint and Test with Node.js 18 on macos-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 18 on windows-latest
🔇 Additional comments (2)
src/index.ts (2)
103-105: Double-check the CLI flag – npm recognises--arch, not--cpuAccording to npm’s current CLI docs the correct flag for overriding architecture is
--arch=<arch>.
--cpuis not listed and may silently be ignored, meaning thewasm32-wasipackage could still install for the host architecture and break at runtime.Please verify the required flag and adjust accordingly:
- target === WASM32_WASI ? ` --cpu=${WASM32}` : '' + target === WASM32_WASI ? ` --arch=${WASM32}` : ''If
--cpuis intentional (e.g. for a custom npm fork) add a comment with a reference explaining why, to avoid future regressions.
243-244:✅ Verification successful
Ensure all call-sites were updated to pass
target
installUsingNPM()gained a parameter and this call was adapted, but others (internal scripts/tests) might still use the old signature.Update any remaining occurrences to avoid runtime
TypeError: expected 6 arguments but got 5.
🏁 Script executed:
# quick grep to verify rg -n "installUsingNPM\(" --glob '!dist/**'Length of output: 365
All
installUsingNPMcall-sites updated
Ranrg -n "installUsingNPM(" --glob "!dist/**"and confirmed every invocation now passes six arguments (includingtarget). No further changes needed.
related unrs/unrs-resolver#95
Summary by CodeRabbit
wasm32-wasitarget architecture, enabling correct installation when using the--cpu=wasm32option.wasm32-wasiCPU target.