feat: support reading package.json from npm registry#21
Conversation
🦋 Changeset detectedLatest commit: 5bad3dd 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 |
|
Warning Rate limit exceeded@JounQin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (19)
📒 Files selected for processing (40)
WalkthroughThis update removes the entire legacy implementation from the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI/Script
participant checkAndPreparePackage
participant LocalPackage
participant NpmRegistry
User->>CLI/Script: Run postinstall/prepare script
CLI/Script->>checkAndPreparePackage: Call with (pkgName, version?, checkVersion?)
alt pkgName is string
checkAndPreparePackage->>LocalPackage: Try require(package.json)
alt Success
checkAndPreparePackage->>checkAndPreparePackage: Use loaded package.json
else Failure
checkAndPreparePackage->>NpmRegistry: Fetch package metadata (if version provided)
NpmRegistry-->>checkAndPreparePackage: Return package.json
end
else pkgName is object
checkAndPreparePackage->>checkAndPreparePackage: Use provided package.json
end
checkAndPreparePackage->>checkAndPreparePackage: Validate and prepare native deps
Assessment against linked issues
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:
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 updates the package installation and preparation flow by extending checkAndPreparePackage to fall back to fetching package.json from the npm registry when local resolution fails, thereby enhancing support for yarn/pnpm in webcontainer environments.
- Updates function overloads and fallback logic in src/index.ts and lib/index.js.
- Adjusts CLI argument parsing and test cases to align with the revised API.
- Updates Yarn configuration and plugin files alongside minor CI adjustments.
Reviewed Changes
Copilot reviewed 19 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/fixtures/yarn/postinstall.js | Adds a postinstall script invoking the updated API |
| test/fixtures/yarn/.yarnrc.yml | Configures Yarn with plugin and release settings |
| test/fixtures/yarn/plugins/plugin-prepare-lifecycle.cjs | Introduces a plugin to hook into the prepare lifecycle |
| test/fixtures.spec.ts | Adds tests for yarn and pnpm behavior |
| test/basic.spec.ts | Updates tests to conform to the new overload signatures |
| src/index.ts | Refactors checkAndPreparePackage to support registry fallback |
| src/helpers.ts | Adjusts version checking logic within getNapiInfoFromPackageJson |
| src/cli.ts | Modifies CLI parameter handling for better cross-tool support |
| lib/index.js | Mirrors src/index.ts changes in the compiled JS implementation |
| lib/helpers.js | Updates version consistency checks in helper functions |
| lib/cli.js | Revises CLI parameter mapping |
| .github/workflows/ci.yml | Updates CI steps to enable corepack before npm ci |
Files not reviewed (6)
- .prettierignore: Language not supported
- package.json: Language not supported
- test/fixtures/pnpm/package.json: Language not supported
- test/fixtures/pnpm/pnpm-lock.yaml: Language not supported
- test/fixtures/yarn/.gitignore: Language not supported
- test/fixtures/yarn/package.json: Language not supported
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Deploy preview for napi-postinstall ready! ✅ Preview Built with commit 5bad3dd. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
test/fixtures/yarn/package.json (1)
1-11: Addprivatefield and unify script usage.
Fixture packages should be private to avoid publishing. For consistency, consider using the same hook (postinstallvsprepare) across Yarn/pnpm fixtures.{ + "private": true, "name": "napi-postinstall-yarn", "packageManager": "yarn@4.9.1", "scripts": { - "prepare": "node ./postinstall.js" + "postinstall": "node ./postinstall.js" }, "devDependencies": { "napi-postinstall": "link:../../..", "rollup": "^4.40.0" } }🧰 Tools
🪛 GitHub Actions: Package Size Report
[error] 1-1: Failed to install dependencies during npm install step.
test/fixtures.spec.ts (3)
7-16: Consider adding a timeout for yarn command execution.The test correctly verifies that the yarn command completes without errors. However, in CI environments, the command might take longer to complete, especially during first-time runs.
- it('should just work', () => + it('should just work', { timeout: 60000 }, () => expect( exec('yarn', [], { nodeOptions: { cwd: path.resolve(fixtures, 'yarn'), }, }), ).resolves.not.toThrow())
18-27: Consider adding a timeout for pnpm command execution.Same as with the yarn test, consider adding a timeout to prevent CI failures on slower environments.
- it('should just work', () => + it('should just work', { timeout: 60000 }, () => expect( exec('pnpm', ['install'], { nodeOptions: { cwd: path.resolve(fixtures, 'pnpm'), }, }), ).resolves.not.toThrow())
1-28: Enhance test suite with more detailed assertions.The current tests verify that commands don't throw errors, which is a good start. Consider enhancing them to verify that the actual functionality works as expected after package installation.
You could verify that:
- The expected files were created
- The package.json was correctly processed
- The postinstall script executed successfully
Example:
it('should install and process correctly', async () => { await exec('yarn', [], { nodeOptions: { cwd: path.resolve(fixtures, 'yarn'), }, }); // Verify expected files exist const nodeModulesExists = fs.existsSync( path.resolve(fixtures, 'yarn', 'node_modules') ); expect(nodeModulesExists).toBe(true); // Additional verification logic here });lib/index.js (2)
103-109: Consider adding request timeout and error handling for network fetch.When fetching from the npm registry, there's no timeout handling which could lead to hanging requests in poor network conditions. Additionally, the error handling could be more specific about network-related issues.
const pkg = packageNameOrPackageJson; -const packageJsonBuffer = await fetch(`${(0, helpers_js_1.getGlobalNpmRegistry)()}${pkg}/${versionOrCheckVersion}`); -packageJson = JSON.parse(packageJsonBuffer.toString('utf8')); +try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30 second timeout + + const packageJsonBuffer = await fetch( + `${(0, helpers_js_1.getGlobalNpmRegistry)()}${pkg}/${versionOrCheckVersion}`, + { signal: controller.signal } + ); + clearTimeout(timeoutId); + packageJson = JSON.parse(packageJsonBuffer.toString('utf8')); +} catch (err) { + if (err.name === 'AbortError') { + throw new Error(`Timeout while fetching package.json for "${pkg}@${versionOrCheckVersion}" from npm registry.`); + } + if (err instanceof SyntaxError) { + throw new Error(`Invalid JSON in package.json for "${pkg}@${versionOrCheckVersion}".`); + } + throw new Error(`Failed to fetch package.json for "${pkg}@${versionOrCheckVersion}": ${(0, helpers_js_1.getErrorMessage)(err)}`); +}Note: This would require modifying the
fetchfunction to accept AbortSignal, which may require additional changes.
96-163: Consider implementing caching for fetched package.json files.For improved performance in scenarios with multiple calls to the same package, consider implementing a simple cache for fetched package.json files. This could significantly reduce network requests and improve performance.
You could implement this by adding a simple in-memory cache at the module level:
// Add at the top of the file const packageJsonCache = new Map(); // In the checkAndPreparePackage function // ... const cacheKey = `${pkg}@${versionOrCheckVersion}`; if (packageJsonCache.has(cacheKey)) { packageJson = packageJsonCache.get(cacheKey); } else { const packageJsonBuffer = await fetch(`${(0, helpers_js_1.getGlobalNpmRegistry)()}${pkg}/${versionOrCheckVersion}`); packageJson = JSON.parse(packageJsonBuffer.toString('utf8')); packageJsonCache.set(cacheKey, packageJson); } // ...src/index.ts (2)
190-191: Consider renaming the parameter for clarityThe dual-purpose parameter
versionOrCheckVersionserves different roles depending on the calling context, which could be confusing for maintainers. Consider renaming it to something that better indicates its dual nature, or restructure the implementation to avoid this pattern.- versionOrCheckVersion?: boolean | string, + versionOrCheckVersionFlag?: boolean | string,
208-213: Consider adding timeout for fetch operationThe current implementation doesn't specify a timeout for the npm registry fetch operation. In poor network conditions, this could cause the function to hang indefinitely. Consider adding a timeout option to the fetch call.
- const packageJsonBuffer = await fetch( - `${getGlobalNpmRegistry()}${pkg}/${versionOrCheckVersion}`, - ) + // Add timeout to prevent hanging in poor network conditions + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); // 30 seconds timeout + try { + const packageJsonBuffer = await fetch( + `${getGlobalNpmRegistry()}${pkg}/${versionOrCheckVersion}`, + { signal: controller.signal } + ); + clearTimeout(timeoutId); + } catch (error) { + clearTimeout(timeoutId); + throw new Error(`Failed to fetch package info from npm registry: ${getErrorMessage(error)}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
lib/cli.js.mapis excluded by!**/*.maplib/helpers.js.mapis excluded by!**/*.maplib/index.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/pnpm/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltest/fixtures/yarn/.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**test/fixtures/yarn/.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**test/fixtures/yarn/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (19)
.changeset/big-phones-chew.md(1 hunks).changeset/soft-cooks-stare.md(1 hunks).github/workflows/ci.yml(1 hunks).prettierignore(1 hunks)lib/cli.js(1 hunks)lib/helpers.js(1 hunks)lib/index.d.ts(1 hunks)lib/index.js(1 hunks)package.json(2 hunks)src/cli.ts(1 hunks)src/helpers.ts(1 hunks)src/index.ts(1 hunks)test/basic.spec.ts(1 hunks)test/fixtures.spec.ts(1 hunks)test/fixtures/pnpm/package.json(1 hunks)test/fixtures/yarn/.gitignore(1 hunks)test/fixtures/yarn/.yarnrc.yml(1 hunks)test/fixtures/yarn/package.json(1 hunks)test/fixtures/yarn/postinstall.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/cli.ts (1)
src/index.ts (1)
checkAndPreparePackage(188-301)
test/fixtures/yarn/postinstall.js (1)
src/index.ts (1)
checkAndPreparePackage(188-301)
test/basic.spec.ts (1)
src/index.ts (1)
checkAndPreparePackage(188-301)
🪛 GitHub Actions: Package Size Report
test/fixtures/pnpm/package.json
[error] 1-1: Failed to install dependencies during npm install step.
test/fixtures/yarn/package.json
[error] 1-1: Failed to install dependencies during npm install step.
package.json
[error] 1-1: Failed to install dependencies during npm install step.
🔇 Additional comments (24)
.prettierignore (1)
1-2: Appropriate Prettier ignore updatesThe additions of
.yarn,.pnp.*, andpnpm-lock.yamlensure that Yarn v2+ and pnpm-related files are excluded from formatting. This aligns with the multi–package-manager support and prevents unintended diffs in lockfiles and PnP artifacts.Also applies to: 6-6
test/fixtures/yarn/.gitignore (1)
1-5: Verify.pnp.*ignore semanticsThe pattern
.yarn/*with exceptions correctly tracks patches/plugins/releases, but the line.pnp.*will ignore all PnP manifest files. If your intent is to commit.pnp.*files (common in Yarn PnP setups), change this to!.pnp.*. Otherwise, confirm that ignoring these files is desired.test/fixtures/yarn/.yarnrc.yml (1)
1-9: Yarn PnP fixture configuration looks soundThe
.yarnrc.ymldisables telemetry, loads the prepare-lifecycle plugin with the correct checksum and path, and pinsyarnPathtoyarn-4.9.1.cjs. This matches the fixture’s requirements for runningcheckAndPreparePackage.package.json (1)
57-57: Bumped devDependencies to support new fixturesDevelopment dependencies for ESLint plugins, TypeScript types,
react-router-dom,stylelint,tinyexec, andvitehave been updated to their latest patch versions to power the new YAML/PNPM fixtures and tests. Theexports["./package.json"]entry also enables direct consumption of metadata from the npm registry. These changes align with the PR’s objectives.Also applies to: 61-61, 65-65, 82-82, 89-90, 93-93
.changeset/big-phones-chew.md (1)
1-6: Changeset file structure is correct and documents the feature.
The frontmatter and summary succinctly capture the addition of npm registry fallback forpackage.json..changeset/soft-cooks-stare.md (1)
1-6: Changeset file is well-formed and describes package manager support.
This succinctly records the new Yarn/pnpm integration in webcontainer environments.lib/cli.js (1)
5-5: Enhanced CLI argument handling.The CLI now accepts a third argument for version checking (via process.argv[4]), aligning with the updated
checkAndPreparePackagefunction signature. This allows the tool to support reading package information from the npm registry when local resolution fails, improving compatibility with package managers like Yarn and pnpm.src/cli.ts (2)
3-3: Removed conditional import.Removed the import of
isNpmsince the function is now called unconditionally.
5-9: Improved package manager compatibility.The function call is now unconditional (previously guarded by
isNpm()) and accepts three parameters instead of two:
process.argv[2]- package nameprocess.argv[3]- version (new parameter)- Boolean flag derived from
process.argv[4]- check version flagThis change allows the tool to work with any package manager by fetching metadata from the npm registry when local resolution fails.
src/helpers.ts (2)
85-88: Relaxed version checking for better compatibility.The error for missing package versions is now removed, allowing the function to continue silently when a package version is missing for a target. This improves compatibility with different package managers that might structure dependencies differently.
90-95: Conditional version consistency check.The version mismatch error is now only thrown when
checkVersionis explicitly set to true. This makes the function more lenient by default while still allowing strict validation when needed.test/basic.spec.ts (4)
6-7: Updated test to match new function signature.The test for
rollupnow explicitly passesundefinedas the version argument andtrueas thecheckVersionflag, matching the updated function signature insrc/index.ts.
10-12: Updated test for @swc/core.Similarly, the test for
@swc/corenow uses the three-argument function signature, explicitly passingundefinedfor the version andtruefor the version check flag.
14-16: Updated test for unrs-resolver.The test for
unrs-resolveralso adopts the new function signature, ensuring consistent testing across different packages.
18-22: Verification: existing error case remains unchanged.The test for
napi-postinstallstill expects the same error message, confirming that the core validation of NAPI support is preserved despite the changes to version handling.test/fixtures.spec.ts (1)
1-6: LGTM: Clean imports and setup.The imports and fixtures path setup look good. Using
path.resolveensures proper path resolution regardless of the working directory.lib/index.d.ts (1)
4-5: LGTM: Clear function overloads for better type safety.The function overloads are well-defined and provide good type safety for both use cases:
- When providing a package name string with optional version and checkVersion params
- When providing a complete PackageJson object with optional checkVersion param
This improves the API's flexibility while maintaining type safety.
lib/helpers.js (2)
67-69: LGTM: Improved error handling for missing package versions.This change makes the function more resilient by skipping iterations with missing package versions rather than throwing errors immediately. This is essential for supporting alternative package managers like Yarn and pnpm which may have different structures.
71-72: LGTM: Conditional version consistency check.Adding the
checkVersioncondition for version consistency errors is a good improvement. This provides more flexibility while still ensuring version consistency when explicitly required.lib/index.js (1)
96-117: LGTM: Enhanced package resolution with npm registry fallback.The function now supports both package name strings and package.json objects, with a fallback to fetch from the npm registry when local resolution fails. This significantly improves compatibility with various package managers like Yarn and pnpm.
The type checking and error messages are clear and helpful.
src/index.ts (4)
178-186: Good API enhancement with function overloadsThe addition of function overloads provides a more flexible API that can handle both package name strings and fully loaded PackageJson objects. This is a clean approach that maintains backward compatibility while adding new functionality.
195-214: Robust fallback mechanism for package resolutionThe implementation correctly handles the case when local package resolution fails (common with pnpm, Yarn v2+) by fetching from the npm registry. The error handling is appropriate, requiring a version string for the fallback path and providing a clear error message if it's not available.
215-223: Correct handling of parameter reassignmentThe code correctly handles parameter reassignment when the second overload is used, ensuring the checkVersion flag is properly set regardless of which overload signature is used.
201-206: 💡 Verification agent🧩 Analysis chain
Verify error handling for network failures
While the TypeError for missing version is handled well, the code should also handle network failures when fetching from the npm registry. Make sure errors from the fetch operation are properly propagated with meaningful messages.
🏁 Script executed:
#!/bin/bash # Check how fetch errors are handled in other parts of the codebase grep -r "fetch.*catch" --include="*.ts" .Length of output: 43
🏁 Script executed:
#!/bin/bash # Search for all fetch calls to inspect error handling rg --context 3 'fetch(' --glob '*.ts' # Search for references to "registry" to locate npm registry-related logic rg -i 'registry' --glob '*.ts'Length of output: 1233
🏁 Script executed:
#!/bin/bash # List all fetch usages with surrounding context to check error handling rg --context 3 -n 'fetch' --glob '*.ts'Length of output: 1253
🏁 Script executed:
#!/bin/bash # Display the fetch implementation sed -n '1,120p' src/index.ts # Display usage around npm registry fetch (lines ~180-260) sed -n '180,260p' src/index.tsLength of output: 6881
Enhance fetch error handling with URL and status context
The current fetch function correctly rejects on non-200 status codes and network errors, but the error messages lack the URL and detailed context. Let’s improve this in src/index.ts within the fetch implementation:
• Location: src/index.ts, lines 22–35
• Change non-200 rejection to include the URL and status code
• Wrap network errors to prefix with the URLProposed diff:
function fetch(url: string) { return new Promise<Buffer>((resolve, reject) => { https .get(url, res => { - if (res.statusCode !== 200) { - return reject(new Error(`Server responded with ${res.statusCode}`)) + if (res.statusCode !== 200) { + return reject(new Error( + `Failed to fetch ${url}: server responded with ${res.statusCode}` + )) } const chunks: Buffer[] = [] res.on('data', (chunk: Buffer) => chunks.push(chunk)) res.on('end', () => resolve(Buffer.concat(chunks))) }) - .on('error', reject) + .on('error', err => + reject(new Error(`Network error when fetching ${url}: ${err.message}`)) + ) }) }This ensures any failure surfaces with a clear, actionable message.
Likely an incorrect or invalid review comment.
0d37b5e to
58a6013
Compare
commit: |
📊 Package size report 3%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
===========================================
- Coverage 55.88% 39.02% -16.87%
===========================================
Files 6 6
Lines 467 492 +25
Branches 67 55 -12
===========================================
- Hits 261 192 -69
- Misses 206 300 +94 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
409fc51 to
d7c7c74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/index.ts (1)
208-213: Security consideration for package metadata fetching.When fetching package metadata from the npm registry, consider implementing a timeout to prevent hanging HTTP requests and rate limiting to prevent potential abuse.
You might want to enhance the fetch operation with a timeout:
- const packageJsonBuffer = await fetch( - `${getGlobalNpmRegistry()}${pkg}/${versionOrCheckVersion}`, - ) + // Add timeout to prevent hanging requests + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout + try { + const packageJsonBuffer = await fetch( + `${getGlobalNpmRegistry()}${pkg}/${versionOrCheckVersion}`, + { signal: controller.signal } + ) + } finally { + clearTimeout(timeoutId); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**lib/cli.js.mapis excluded by!**/*.maplib/constants.js.mapis excluded by!**/*.maplib/helpers.js.mapis excluded by!**/*.maplib/index.js.mapis excluded by!**/*.maplib/target.js.mapis excluded by!**/*.maplib/types.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/npm-10/package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/npm/package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/pnpm/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltest/fixtures/yarn-pnp/.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**test/fixtures/yarn-pnp/.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**test/fixtures/yarn-pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.locktest/fixtures/yarn/.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**test/fixtures/yarn/.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**test/fixtures/yarn/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
.changeset/big-phones-chew.md(1 hunks).changeset/soft-cooks-stare.md(1 hunks).github/workflows/autofix.yml(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/pkg-pr-new.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/size-limit.yml(1 hunks).gitignore(1 hunks).prettierignore(1 hunks).simple-git-hooks.mjs(1 hunks).yarnrc.yml(1 hunks)lib/cli.d.ts(0 hunks)lib/cli.js(0 hunks)lib/constants.d.ts(0 hunks)lib/constants.js(0 hunks)lib/helpers.d.ts(0 hunks)lib/helpers.js(0 hunks)lib/index.d.ts(0 hunks)lib/index.js(0 hunks)lib/target.d.ts(0 hunks)lib/target.js(0 hunks)lib/types.d.ts(0 hunks)lib/types.js(0 hunks)package.json(4 hunks)src/cli.ts(1 hunks)src/helpers.ts(1 hunks)src/index.ts(1 hunks)test/basic.spec.ts(1 hunks)test/fixtures.spec.ts(1 hunks)test/fixtures/npm-10/package.json(1 hunks)test/fixtures/npm/package.json(1 hunks)test/fixtures/pnpm/package.json(1 hunks)test/fixtures/yarn-pnp/.gitignore(1 hunks)test/fixtures/yarn-pnp/.yarnrc.yml(1 hunks)test/fixtures/yarn-pnp/package.json(1 hunks)test/fixtures/yarn-pnp/postinstall.js(1 hunks)test/fixtures/yarn/.gitignore(1 hunks)test/fixtures/yarn/.yarnrc.yml(1 hunks)test/fixtures/yarn/package.json(1 hunks)test/fixtures/yarn/postinstall.js(1 hunks)
💤 Files with no reviewable changes (12)
- lib/types.js
- lib/cli.d.ts
- lib/cli.js
- lib/index.js
- lib/target.d.ts
- lib/index.d.ts
- lib/constants.js
- lib/constants.d.ts
- lib/types.d.ts
- lib/target.js
- lib/helpers.d.ts
- lib/helpers.js
✅ Files skipped from review due to trivial changes (5)
- test/fixtures/yarn/.gitignore
- test/fixtures/yarn/.yarnrc.yml
- test/fixtures/yarn-pnp/package.json
- .yarnrc.yml
- test/fixtures/npm-10/package.json
🚧 Files skipped from review as they are similar to previous changes (17)
- .changeset/big-phones-chew.md
- .changeset/soft-cooks-stare.md
- .prettierignore
- test/fixtures/yarn-pnp/.yarnrc.yml
- test/fixtures/yarn/package.json
- .gitignore
- test/fixtures/yarn-pnp/.gitignore
- test/fixtures/yarn/postinstall.js
- .github/workflows/ci.yml
- test/fixtures/pnpm/package.json
- src/helpers.ts
- test/basic.spec.ts
- .simple-git-hooks.mjs
- test/fixtures/yarn-pnp/postinstall.js
- test/fixtures/npm/package.json
- test/fixtures.spec.ts
- package.json
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/cli.ts
[warning] 3-3: src/cli.ts#L3
Added line #L3 was not covered by tests
[warning] 5-9: src/cli.ts#L5-L9
Added lines #L5 - L9 were not covered by tests
src/index.ts
[warning] 202-214: src/index.ts#L202-L214
Added lines #L202 - L214 were not covered by tests
[warning] 216-223: src/index.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (3)
- 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
🔇 Additional comments (16)
.github/workflows/size-limit.yml (2)
22-22: Consistent package manager caching
Switching the Node setup cache from npm to Yarn aligns with your migration to Yarn; this should speed up dependency installation.
25-25: Use immutable Yarn installation
Replacingnpm ciwithyarn --immutableensures that dependencies are installed strictly according to the lockfile, matching best practices for reproducible builds..github/workflows/autofix.yml (3)
25-25: Enable Yarn caching for faster CI runs
Updating the cache option toyarnis correct to leverage cached modules and speed up the workflow.
28-28: Install dependencies with Yarn
Usingyarn --immutablehere enforces the lockfile and aligns with the repository migration; good addition.
31-31: Update formatting command to Yarn
Switching fromnpm run formattoyarn formatis appropriate, assuming theformatscript is defined inpackage.json..github/workflows/release.yml (4)
32-32: Switch Node cache to Yarn
Changingcache: npmtocache: yarnis consistent with the overall package manager migration.
35-35: Adopt Yarn immutable install
Replacingnpm ciwithyarn --immutableensures deterministic installs, which is crucial for reliable releases.
42-42: Use Yarn for publishing
Configuring thepublishcommand asyarn releasecorrectly replacesnpm run release. Make sure thereleasescript exists and performs the expected publish logic.
43-43: Use Yarn for version bump
Adjusting theversioncommand toyarn run versionmirrors the previous npm-based bump. Ensure this script has the proper side effects in your Changesets workflow..github/workflows/pkg-pr-new.yml (4)
22-22: Leverage Yarn cache
Switching the Node.js cache context toyarncorrectly aligns with the migration to Yarn across workflows.
25-25: Install dependencies reproducibly
Usingyarn --immutablereplacesnpm ciin enforcing lockfile consistency; good for build determinism.
28-28: Update build step to Yarn
Runningyarn buildinstead ofnpm run buildis the expected transition; ensure thebuildscript is defined and invoked correctly.
31-31: Use Yarn dlx for CLI execution
Switching fromnpxtoyarn dlxto runpkg-pr-newis a best practice for Yarn deployments and avoids potential PnP resolution issues.src/index.ts (3)
178-186: Improved TypeScript function overload signatures.The explicit function overloads provide better type safety and clearer API documentation for the two supported calling patterns. This is good TypeScript practice.
215-223: Handling for direct PackageJson object input.The implementation now gracefully handles receiving a PackageJson object directly, which increases flexibility. The parameter assignment and type check are correctly implemented.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-223: src/index.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
202-206: Appropriate error handling when version is missing.The error message is clear and descriptive when the package.json can't be loaded locally and no version is provided. This provides good developer experience by explaining exactly what went wrong and how to fix it.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-214: src/index.ts#L202-L214
Added lines #L202 - L214 were not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/index.ts (1)
195-214: Missing Test Coverage for npm Registry FallbackThe new fallback logic for fetching from the npm registry isn't currently covered by tests according to static analysis. Consider adding unit tests that verify both successful registry fetches and error handling when version is missing.
#!/bin/bash # Check if any tests verify the npm registry fallback functionality find test -type f -name "*.ts" -o -name "*.js" | xargs grep -l "registry" | xargs cat | grep -C 5 "registry"🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-214: src/index.ts#L202-L214
Added lines #L202 - L214 were not covered by tests
🧹 Nitpick comments (1)
src/index.ts (1)
202-206: Consider enhancing error message with examplesThe error message could be more helpful by providing an example of how to correctly specify the version.
- throw new TypeError( - `Failed to load \`${PACKAGE_JSON}\` from "${packageNameOrPackageJson}", please provide a version.`, - ) + throw new TypeError( + `Failed to load \`${PACKAGE_JSON}\` from "${packageNameOrPackageJson}", please provide a version. Example: checkAndPreparePackage("${packageNameOrPackageJson}", "1.0.0")`, + )🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-214: src/index.ts#L202-L214
Added lines #L202 - L214 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**lib/cli.js.mapis excluded by!**/*.maplib/constants.js.mapis excluded by!**/*.maplib/helpers.js.mapis excluded by!**/*.maplib/index.js.mapis excluded by!**/*.maplib/target.js.mapis excluded by!**/*.maplib/types.js.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/npm-10/package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/npm/package-lock.jsonis excluded by!**/package-lock.jsontest/fixtures/pnpm/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamltest/fixtures/yarn-pnp/.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**test/fixtures/yarn-pnp/.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**test/fixtures/yarn-pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.locktest/fixtures/yarn/.yarn/plugins/plugin-prepare-lifecycle.cjsis excluded by!**/.yarn/**test/fixtures/yarn/.yarn/releases/yarn-4.9.1.cjsis excluded by!**/.yarn/**test/fixtures/yarn/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (40)
.changeset/big-phones-chew.md(1 hunks).changeset/soft-cooks-stare.md(1 hunks).github/workflows/autofix.yml(1 hunks).github/workflows/ci.yml(1 hunks).github/workflows/pkg-pr-new.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/size-limit.yml(1 hunks).gitignore(1 hunks).prettierignore(1 hunks).simple-git-hooks.mjs(1 hunks).yarnrc.yml(1 hunks)lib/cli.d.ts(0 hunks)lib/cli.js(0 hunks)lib/constants.d.ts(0 hunks)lib/constants.js(0 hunks)lib/helpers.d.ts(0 hunks)lib/helpers.js(0 hunks)lib/index.d.ts(0 hunks)lib/index.js(0 hunks)lib/target.d.ts(0 hunks)lib/target.js(0 hunks)lib/types.d.ts(0 hunks)lib/types.js(0 hunks)package.json(4 hunks)src/cli.ts(1 hunks)src/helpers.ts(1 hunks)src/index.ts(1 hunks)test/basic.spec.ts(1 hunks)test/fixtures.spec.ts(1 hunks)test/fixtures/npm-10/package.json(1 hunks)test/fixtures/npm/package.json(1 hunks)test/fixtures/pnpm/package.json(1 hunks)test/fixtures/yarn-pnp/.gitignore(1 hunks)test/fixtures/yarn-pnp/.yarnrc.yml(1 hunks)test/fixtures/yarn-pnp/package.json(1 hunks)test/fixtures/yarn-pnp/postinstall.js(1 hunks)test/fixtures/yarn/.gitignore(1 hunks)test/fixtures/yarn/.yarnrc.yml(1 hunks)test/fixtures/yarn/package.json(1 hunks)test/fixtures/yarn/postinstall.js(1 hunks)
💤 Files with no reviewable changes (12)
- lib/types.js
- lib/cli.d.ts
- lib/target.d.ts
- lib/cli.js
- lib/index.d.ts
- lib/index.js
- lib/constants.js
- lib/target.js
- lib/helpers.d.ts
- lib/types.d.ts
- lib/constants.d.ts
- lib/helpers.js
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/size-limit.yml
- test/fixtures/yarn/.yarnrc.yml
🚧 Files skipped from review as they are similar to previous changes (24)
- .changeset/soft-cooks-stare.md
- .changeset/big-phones-chew.md
- test/fixtures/yarn/.gitignore
- test/fixtures/pnpm/package.json
- .prettierignore
- .gitignore
- .yarnrc.yml
- test/fixtures/yarn/postinstall.js
- .github/workflows/release.yml
- test/basic.spec.ts
- test/fixtures/yarn-pnp/.gitignore
- .github/workflows/pkg-pr-new.yml
- test/fixtures/yarn-pnp/postinstall.js
- .github/workflows/autofix.yml
- test/fixtures/yarn-pnp/package.json
- test/fixtures.spec.ts
- test/fixtures/yarn-pnp/.yarnrc.yml
- src/helpers.ts
- test/fixtures/npm/package.json
- test/fixtures/yarn/package.json
- .simple-git-hooks.mjs
- test/fixtures/npm-10/package.json
- package.json
- .github/workflows/ci.yml
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/index.ts (3)
src/types.ts (1)
PackageJson(28-33)src/constants.ts (2)
require(7-9)PACKAGE_JSON(19-19)src/helpers.ts (1)
getGlobalNpmRegistry(9-18)
🪛 GitHub Check: codecov/patch
src/cli.ts
[warning] 3-3: src/cli.ts#L3
Added line #L3 was not covered by tests
[warning] 5-9: src/cli.ts#L5-L9
Added lines #L5 - L9 were not covered by tests
src/index.ts
[warning] 202-214: src/index.ts#L202-L214
Added lines #L202 - L214 were not covered by tests
[warning] 216-223: src/index.ts#L216-L223
Added lines #L216 - L223 were 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 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 (3)
src/cli.ts (1)
3-9:Details
❓ Verification inconclusive
Streamlined CLI implementation for package manager agnostic operation
The CLI implementation has been simplified by removing the conditional check on
isNpm()and now unconditionally callscheckAndPreparePackagewith explicitly defined parameters. This aligns with the PR objective of supporting readingpackage.jsonfrom npm registry for better yarn/pnpm compatibility.The boolean handling in the third parameter is clear and intuitive, supporting multiple affirmative values ('1', 'check', 'true', 'yes').
🏁 Script executed:
#!/bin/bash # Verify that the e2e tests cover the CLI functionality with different package managers find test -type f -name "*.ts" -o -name "*.js" | xargs grep -l "fixtures" | xargs cat | grep -C 5 "checkAndPreparePackage"Length of output: 120
Ensure CLI e2e coverage across package managers
I searched the test suite for any invocations of
checkAndPreparePackageand found no matches. Please verify that your end-to-end or integration tests exercise the new CLI behavior under npm, Yarn, and PNPM configurations to avoid regressions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 3-3: src/cli.ts#L3
Added line #L3 was not covered by tests
[warning] 5-9: src/cli.ts#L5-L9
Added lines #L5 - L9 were not covered by testssrc/index.ts (2)
178-186: LGTM: Well-defined function overloads improve type safetyAdding explicit function overloads for the
checkAndPreparePackagefunction provides better type safety and clearer API documentation. This makes it easier for consumers to understand the expected parameters and return types.
189-223:Details
✅ Verification successful
Implementation now supports package manager agnostic operation
The refactored implementation properly handles both string package names and direct PackageJson objects, with a clear fallback mechanism for fetching package.json from the npm registry when local resolution fails. This directly addresses the PR objective to support yarn and pnpm in webcontainer environments.
🏁 Script executed:
#!/bin/bash # Check if there are e2e tests that validate the package manager compatibility echo "Looking for e2e tests that exercise different package managers:" find test/fixtures -type d -maxdepth 1 | sort echo "\nChecking for package manager specific test configurations:" grep -r "packageManager" test/fixtures --include="*.json" || echo "No explicit package manager configurations found"Length of output: 823
Package manager compatibility verified via E2E fixtures
The existing test suite already covers npm, pnpm, and Yarn (including PnP) under
test/fixtures(withpackageManagerfields in eachpackage.json), ensuring the new fallback logic is exercised. No further changes required.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 202-214: src/index.ts#L202-L214
Added lines #L202 - L214 were not covered by tests
[warning] 216-223: src/index.ts#L216-L223
Added lines #L216 - L223 were not covered by tests
support yarn/pnpm seamlessly for webcontainer
support
yarn/pnpmseamlessly forwebcontainerclose #17
close #20
close unrs/unrs-resolver#101
Summary by CodeRabbit
Summary by CodeRabbit
New Features
package.jsonfrom the npm registry.Bug Fixes
Chores
.prettierignoreand.gitignoreto better manage ignored files.Tests
Refactor
Documentation