fix: give up patching yarn pnp automatically which is impossible#25
fix: give up patching yarn pnp automatically which is impossible#25JounQin merged 4 commits intoun-ts:mainfrom
yarn pnp automatically which is impossible#25Conversation
🦋 Changeset detectedLatest commit: ca21c84 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 17 minutes and 25 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 (2)
📒 Files selected for processing (13)
WalkthroughThe changes update configuration and dependency management in two Yarn-based test fixture directories. Both Changes
Suggested labels
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 (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/fixtures.spec.ts (1)
9-18: Consider consistent test validation across all package managers.While the Yarn tests have been updated to check for empty stderr output, the npm and pnpm tests still only verify that no exceptions are thrown. For consistency and to catch similar issues across all package managers, consider updating these tests to follow the same pattern as the Yarn tests.
describe('npm fixture', () => { - it('should just work', () => - expect( - exec('npm', ['install'], { - nodeOptions: { - cwd: path.resolve(fixtures, 'npm'), - }, - }), - ).resolves.not.toThrow()) + it('should just work', async () => { + const res = await exec('npm', ['install'], { + nodeOptions: { + cwd: path.resolve(fixtures, 'npm'), + }, + }) + expect(res.stderr).toEqual('') + }) })Also applies to: 20-29, 31-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/fixtures.spec.ts(1 hunks)
🔇 Additional comments (2)
test/fixtures.spec.ts (2)
42-50: Enhanced test validation for Yarn fixture.The test has been improved to explicitly check for empty stderr output rather than just checking if the command doesn't throw an exception. This addresses the issue mentioned in the PR objectives where Yarn doesn't exit with a non-zero status code for certain errors but still emits error messages, which the previous tests weren't catching.
53-61: Improved error detection for Yarn PnP fixture.Similar to the previous test, this change enhances validation by specifically checking for empty stderr output. This is particularly important for the PnP fixture since the PR objectives note that the tests were previously failing to detect PnP-related errors.
I didn't have this issue locally, corepack must be enabled. |
commit: |
|
@mrginglymus Thanks, should we adapt other fixtures at the same time? |
0eb4059 to
c235c1c
Compare
yarn pnp automatically which is impossible
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/fixtures/yarn-pnp/.yarn/patches/rollup-npm-4.40.0-5ee51badc3.patchis excluded by!**/.yarn/**test/fixtures/yarn-pnp/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(1 hunks)src/constants.ts(1 hunks)src/helpers.ts(4 hunks)src/index.ts(10 hunks)test/basic.spec.ts(1 hunks)test/fixtures.spec.ts(1 hunks)test/fixtures/yarn-pnp/.gitignore(1 hunks)test/fixtures/yarn-pnp/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/yarn-pnp/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- test/basic.spec.ts
- package.json
- test/fixtures/yarn-pnp/.gitignore
- src/constants.ts
- test/fixtures.spec.ts
- src/helpers.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/index.ts (4)
src/helpers.ts (4)
errorMessage(280-282)getErrorMessage(272-274)errorLog(276-278)downloadedNodePath(38-41)src/constants.ts (3)
PACKAGE_JSON(21-21)WASM32_WASI(17-17)meta(7-9)test/fixtures/yarn-pnp/postinstall.js (1)
require(1-1)test/fixtures/yarn/postinstall.js (1)
require(1-1)
🔇 Additional comments (12)
src/index.ts (12)
9-9: Consistent import of package metadata from constants.The import now retrieves the full
metaobject from constants.js instead of destructuring specific values, aligning with other parts of the codebase. This change supports the later use ofmeta.namefor path resolution.
12-13: Good addition of error handling helpers.Adding imports for the
errorLoganderrorMessagehelpers standardizes error reporting across the codebase, making error messages more consistent and maintainable.
51-52: Improved error message formatting.Using the
errorMessagehelper ensures consistent error message formatting with the log prefix.
70-70: Standardized error message formatting.Using the
errorMessagehelper here maintains consistency in how errors are presented to users.
73-75: Simplified isNpm function.The redundant boolean casting was removed while maintaining the same functionality. This is a good simplification.
77-82: Added Yarn PnP detection function.This function appropriately checks for both Yarn in the user agent and the presence of the PnP version, which is essential for the Yarn PnP-specific handling added elsewhere in the code.
106-121: Improved error handling for read-only filesystems.This change adds graceful handling for EROFS errors when attempting to create the installation directory. Instead of throwing an error, it now provides a helpful message to the user and continues execution, which improves the robustness of the installation process.
238-238: Standardized error logging in download functions.Replacing console.error calls with the
errorLoghelper ensures consistent formatting of error messages throughout the codebase.Also applies to: 245-245
276-278: Consistent error message formatting.All error messages now use the
errorMessagehelper, ensuring consistent formatting throughout the codebase.Also applies to: 308-310, 399-402
323-326: Improved readability with local variable.Extracting the
isWasm32Wasicheck into a local variable improves code readability and reduces duplicate checks.
369-380: Added robust path resolution with Yarn PnP fallback.The added try-catch block improves robustness by providing a fallback path resolution method when the standard approach fails (e.g., in Yarn PnP environments). This is a good defensive programming practice that ensures the code can handle various environment configurations.
382-382: Standardized error logging for installation attempts.Using the
errorLoghelper for installation logging ensures consistent formatting and improves the user experience by providing clear, standardized error messages.Also applies to: 386-389
9debfbb to
f52c452
Compare
|
Thanks @mrginglymus! |
|
Thanks @JounQin - all working on my end now :) |
Yarn PnP doesn't work with this, but tests claim it does. There are three reasons
linkprotocol doesn't capture the dependency behaviour correctlyyarnin the fixtures directory shows the error, but the tests claim everything is fine.edit: ah the problem is that yarn is not exiting non-zero with this error, just emitting errors.
Summary by CodeRabbit