Switch to pnpm #1702
Switch to pnpm #1702
Conversation
WalkthroughSwaps Yarn/Volta for pnpm across CI, scripts, and docs: CI sets up Node 18 + pnpm, package.json scripts/engines switched to pnpm/Node 18, release hook and .npmignore updated, Jest config adapted for pnpm layout, tests added and a Cypress assertion relaxed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.github/workflows/main.yml(1 hunks).npmignore(1 hunks).release-it.js(1 hunks)CONTRIBUTING.md(3 hunks)RELEASE.md(1 hunks)package.json(2 hunks)
🔇 Additional comments (7)
.npmignore (1)
23-23: LGTM!Correctly excludes pnpm-lock.yaml from npm publishing, consistent with the existing yarn.lock exclusion.
RELEASE.md (1)
34-34: LGTM!Documentation correctly updated to reflect the pnpm migration.
.release-it.js (1)
5-5: LGTM!Correctly updates the release hook to use pnpm for building.
CONTRIBUTING.md (1)
5-49: LGTM!Documentation consistently updated across all sections to reflect pnpm usage. All commands are correct pnpm equivalents.
package.json (2)
21-35: LGTM!All npm scripts correctly updated to use pnpm. The script references are properly chained and will execute as expected.
1-86: pnpm migration is complete.Both requirements have been satisfied:
pnpm-lock.yamlexists (426 KB)yarn.lockhas been removed.github/workflows/main.yml (1)
18-28: LGTM!The CI workflow correctly migrates to pnpm with:
- Updated action versions (checkout@v4, pnpm/action-setup@v4, setup-node@v4)
- Explicit Node.js 18 setup with pnpm caching
- Proper pnpm installation and test commands
The cache configuration will significantly speed up CI runs by caching pnpm's store.
| "node": ">= 18", | ||
| "pnpm": ">= 10" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pnpm version 10 latest stable release 2025
💡 Result:
The latest stable pnpm v10 release is v10.24.0, published on November 27, 2025. [1][2]
Sources:
[1] pnpm GitHub releases (v10.24.0).
[2] pnpm blog — 10.24 release notes.
🌐 Web query:
Node.js 18 LTS status December 2025 end of life
💡 Result:
Short answer: Node.js 18 (LTS) reached official upstream End‑of‑Life on April 30, 2025 — so as of December 4, 2025 it is EOL. [1][2]
Notes:
- Upstream Node.js no longer issues fixes/patches for 18 after April 30, 2025. [1]
- Several vendors and ecosystems have since dropped or limited support (e.g., AWS/Azure tooling timelines); some vendors (e.g., Canonical/Ubuntu Pro) offer extended security maintenance for Node 18 on their platforms. [3][4]
Sources:
- Node.js EOL page. [Node.js — End‑Of‑Life]. [1]
- EndOfLife.so summary for Node.js 18 (EOL: Apr 30, 2025). [2]
- Canonical announcement re: extended Node.js 18 support via Ubuntu Pro. [3]
- AWS blog announcing end of support for Node.js 18 in AWS CDK. [4]
Update Node.js minimum version requirement from 18 to at least 20 LTS.
Node.js 18 reached official End-of-Life on April 30, 2025, and no longer receives upstream security patches or fixes. Specifying Node.js >= 18 as the minimum requirement as of December 2025 exposes users to EOL software. Node.js 20 (LTS) is the recommended minimum version. The pnpm >= 10 requirement is valid; pnpm v10.24.0 is the latest stable release (November 27, 2025).
🤖 Prompt for AI Agents
In package.json around lines 76-77, the engines field currently requires "node":
">= 18" which is EOL; update the minimum Node.js requirement to ">= 20" (Node 20
LTS) and keep "pnpm": ">= 10" as-is; after changing the engines entry, update
any CI/workflow/node-version settings and developer docs that reference Node 18
to Node 20 and run the test suite/lockfile regen to ensure compatibility.
| "pnpm": { | ||
| "onlyBuiltDependencies": [ | ||
| "@parcel/watcher", | ||
| "core-js", | ||
| "cypress" | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the listed packages exist in package.json dependencies
echo "Checking for packages in onlyBuiltDependencies..."
echo ""
for pkg in "@parcel/watcher" "core-js" "cypress"; do
echo "Searching for $pkg:"
cat package.json | jq -r "
(.dependencies // {}) + (.devDependencies // {}) + (.peerDependencies // {}) + (.optionalDependencies // {})
| to_entries[]
| select(.key == \"$pkg\")
| \" Found: \(.key)@\(.value)\"
" || echo " Not found"
done
echo ""
echo "Full onlyBuiltDependencies list:"
jq '.pnpm.onlyBuiltDependencies' package.jsonRepository: shipshapecode/tether
Length of output: 300
Remove non-existent packages from pnpm.onlyBuiltDependencies.
The list includes @parcel/watcher and core-js, neither of which exist in any dependency section of package.json. Only cypress is actually present (as a devDependency). Remove the non-existent packages from the configuration to avoid potential pnpm resolution issues.
🤖 Prompt for AI Agents
package.json lines 79-85: the pnpm.onlyBuiltDependencies array contains entries
that are not present in this package.json; remove "@parcel/watcher" and
"core-js" from that array and leave only "cypress" (or replace with any other
actual dependency names present in your dependencies/devDependencies), then
ensure the resulting JSON is still valid (commas/spacing) so pnpm won't attempt
to resolve non-existent packages.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jest.config.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
| }, | ||
|
|
||
| // Transform sinon ESM module (handle pnpm structure) | ||
| transformIgnorePatterns: [ | ||
| 'node_modules/(?!(.pnpm|sinon))' | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "jest.config.js" -type fRepository: shipshapecode/tether
Length of output: 80
🏁 Script executed:
cat -n jest.config.jsRepository: shipshapecode/tether
Length of output: 1495
🏁 Script executed:
cat -n jest.config.js | sed -n '28,37p'Repository: shipshapecode/tether
Length of output: 374
🌐 Web query:
Jest transformIgnorePatterns negative lookahead regex explanation
💡 Result:
Short explanation — what the negative lookahead does and how Jest uses it
- Pattern: /node_modules/(?!(foo|bar)/)
- (?!(foo|bar)/) is a negative lookahead that asserts “the text immediately after node_modules/ is NOT foo/ or bar/”.
- So the regex matches paths inside node_modules except those whose next path segment is foo or bar.
- In Jest: transformIgnorePatterns is an array of regexes matched against the full file path. If a path matches any pattern it will NOT be transformed. Therefore:
- "/node_modules/(?!(foo|bar)/)" means “ignore (don’t transform) everything in node_modules except node_modules/foo/ and node_modules/bar/” (those two will be transformed because they do not match this ignore pattern). [1][2]
- Important notes:
- The negative lookahead only inspects the characters immediately following the cursor — you must include the trailing slash (or other path boundary) so you only exempt the package folder, e.g. (?!(foo|bar)/).
- If you have other ignore patterns that also match those package paths (e.g. "/bar/"), they will still prevent transformation — overlapping patterns combine by logical OR. [1]
- If you need to whitelist multiple packages, list them together inside the same lookahead: "/node_modules/(?!(pkg-a|pkg-b|pkg-c)/)". [2]
Sources: Jest docs on transformIgnorePatterns and examples; community discussion/examples. [1] [2]
References
[1] Jest docs — transformIgnorePatterns (configuration).
[2] StackOverflow — explanation/examples of transformIgnorePatterns and negative lookahead.
🏁 Script executed:
# Check if there are any test files that use sinon
find . -type f \( -name "*.test.js" -o -name "*.spec.js" \) | head -20Repository: shipshapecode/tether
Length of output: 251
🏁 Script executed:
# Look for sinon imports in test files
rg "sinon" --type js -l | head -20Repository: shipshapecode/tether
Length of output: 137
🏁 Script executed:
# Check package.json for sinon and related dependencies
cat -n package.json | grep -i sinon -A2 -B2Repository: shipshapecode/tether
Length of output: 247
🏁 Script executed:
# Examine how sinon is imported in test files
head -20 test/unit/evented.spec.jsRepository: shipshapecode/tether
Length of output: 603
🏁 Script executed:
# Check if there's a package-lock or pnpm-lock file to see actual dependencies
ls -la | grep -E "(package-lock|pnpm-lock|yarn.lock)"Repository: shipshapecode/tether
Length of output: 130
🏁 Script executed:
# Look for any jest test output or configuration issues
if [ -f "package.json" ]; then
# Extract jest config or test command
cat package.json | grep -A5 -B5 '"test"'
fiRepository: shipshapecode/tether
Length of output: 663
🏁 Script executed:
# Let's verify if the pattern actually works by checking how Jest would interpret it
# Create a test to show what files would match
python3 << 'EOF'
import re
# The pattern as it appears in jest.config.js
pattern = r'node_modules/(?!(.pnpm|sinon))'
# Test paths
test_paths = [
'node_modules/lodash/index.js',
'node_modules/sinon/lib/sinon.js',
'node_modules/sinon-chai/index.js',
'node_modules/.pnpm/[email protected]/node_modules/sinon/lib/sinon.js',
'node_modules/.pnpm/[email protected]/node_modules/lodash/index.js',
]
regex = re.compile(pattern)
print("Pattern:", pattern)
print("\nTest Results (True = IGNORED from transformation, False = TRANSFORMED):")
print("-" * 70)
for path in test_paths:
match = regex.search(path)
status = "IGNORED" if match else "TRANSFORMED"
print(f"{path:<60} {status}")
print("\n" + "="*70)
print("\nWith corrected pattern (trailing slash):")
corrected_pattern = r'node_modules/(?!(.pnpm|sinon)/)'
regex_corrected = re.compile(corrected_pattern)
print("Pattern:", corrected_pattern)
print("\nTest Results (True = IGNORED, False = TRANSFORMED):")
print("-" * 70)
for path in test_paths:
match = regex_corrected.search(path)
status = "IGNORED" if match else "TRANSFORMED"
print(f"{path:<60} {status}")
EOFRepository: shipshapecode/tether
Length of output: 150
Add trailing slash to transformIgnorePatterns regex pattern.
The transformIgnorePatterns pattern is missing a trailing slash in the negative lookahead. It should be:
transformIgnorePatterns: [
'node_modules/(?!(.pnpm|sinon)/)'
]Current pattern node_modules/(?!(.pnpm|sinon)) will incorrectly match package names that start with "sinon" or ".pnpm" (e.g., "sinon-chai"), potentially causing unintended transformations. Per Jest documentation, the trailing slash is required to properly delimit package boundaries.
🤖 Prompt for AI Agents
In jest.config.js around lines 31 to 36, the transformIgnorePatterns negative
lookahead is missing a trailing slash which allows matches like "sinon-chai";
update the pattern to include a trailing slash so it only exempts whole package
directories (e.g., change the entry to use node_modules/(?!(.pnpm|sinon)/) so
the lookahead is properly delimited).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
package.json (2)
76-77: Node.js 18 reached End-of-Life in April 2025.This issue was flagged in a previous review. Node.js 18 no longer receives security patches. Consider updating to
>= 20as the minimum requirement.
79-85:onlyBuiltDependenciescontains packages not in dependencies.This issue was flagged in a previous review.
@parcel/watcherandcore-jsare not present in the dependencies. Onlycypressexists as a devDependency.
🧹 Nitpick comments (1)
test/unit/utils/parents.spec.js (1)
164-179: Test doesn't actually simulategetComputedStylereturning null.The comment mentions creating a scenario where
getComputedStylereturns null (Firefox in display:none iframes), but the test just creates a normal DOM structure. This test only verifies the function doesn't throw for a standard DOM hierarchy, not the edge case described.To properly test this edge case, you'd need to mock
getComputedStyle:it('handles elements where getComputedStyle returns null', () => { testElement = document.createElement('div'); body.appendChild(testElement); const originalGetComputedStyle = window.getComputedStyle; window.getComputedStyle = jest.fn((el) => { if (el === testElement.parentNode) return null; return originalGetComputedStyle(el); }); try { const parents = getScrollParents(testElement); expect(Array.isArray(parents)).toBe(true); } finally { window.getComputedStyle = originalGetComputedStyle; } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)test/unit/shift.spec.js(1 hunks)test/unit/utils/bounds.spec.js(1 hunks)test/unit/utils/parents.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/unit/utils/bounds.spec.js (1)
src/js/utils/bounds.js (3)
removeUtilElements(145-150)getScrollHandleBounds(46-104)getVisibleBounds(111-143)
test/unit/utils/parents.spec.js (1)
src/js/utils/parents.js (3)
parents(8-8)getScrollParents(3-44)getOffsetParent(46-48)
🪛 GitHub Actions: CI Build
test/unit/shift.spec.js
[error] 38-38: 'top' is defined but never used. (no-unused-vars)
🪛 GitHub Check: Tests
test/unit/shift.spec.js
[failure] 38-38:
'left' is defined but never used
[failure] 38-38:
'top' is defined but never used
test/unit/utils/bounds.spec.js
[failure] 88-88:
Avoid calling expect conditionally`
🔇 Additional comments (4)
test/unit/shift.spec.js (1)
1-72: Good test coverage for Shift.position.The test suite comprehensively covers multiple input scenarios: undefined options, string values (single and dual), object values, function values with different return types, and negative values. The test structure is clean and follows Jest conventions.
test/unit/utils/parents.spec.js (1)
1-215: Solid test coverage for parent utilities.The tests comprehensively cover scroll parent detection across various overflow and positioning scenarios. The setup/teardown pattern and manual cleanup for non-testElement nodes are handled correctly.
test/unit/utils/bounds.spec.js (1)
1-191: Comprehensive test coverage for bounds utilities.The test suite covers
getBounds,getScrollHandleBounds,getVisibleBounds,removeUtilElements, and caching behavior effectively. The setup/teardown properly manages DOM elements and cleans up utility elements.package.json (1)
21-35: Script updates for pnpm migration look correct.The script commands have been properly updated to use
pnpminstead of yarn/npm. The URL change fromlocalhostto127.0.0.1in test scripts is a good practice for consistency in CI environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/unit/utils/bounds.spec.js (1)
154-166: Consider strengthening the removal assertion.The test verifies cleanup but uses
toBeLessThanOrEqual, which would pass even if removal didn't occur (whenelementsAfter === elementsBefore).For more precise verification:
- expect(elementsAfter).toBeLessThanOrEqual(elementsBefore); + expect(elementsAfter).toBeLessThan(elementsBefore);Or verify complete cleanup:
- expect(elementsAfter).toBeLessThanOrEqual(elementsBefore); + expect(elementsAfter).toBe(0);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/cypress/integration/scroll.cy.js(1 hunks)test/unit/shift.spec.js(1 hunks)test/unit/utils/bounds.spec.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/shift.spec.js
🧰 Additional context used
🧬 Code graph analysis (1)
test/unit/utils/bounds.spec.js (1)
src/js/utils/bounds.js (3)
removeUtilElements(145-150)getScrollHandleBounds(46-104)getVisibleBounds(111-143)
🔇 Additional comments (6)
test/cypress/integration/scroll.cy.js (1)
27-28: LGTM! Tolerance-based assertion appropriately handles sub-pixel rounding.The change from exact equality to
closeTowith a 1-pixel tolerance is appropriate for handling legitimate sub-pixel rounding differences in browser rendering. The explanatory comment makes the intent clear.test/unit/utils/bounds.spec.js (5)
1-23: LGTM! Well-structured test setup and teardown.The setup properly creates test elements with defined positioning and dimensions, and the teardown ensures proper cleanup for test isolation.
25-49: LGTM! getBounds tests provide good baseline coverage.The tests appropriately verify bounds calculations for regular elements and the document element, including computed properties like right and bottom.
51-106: LGTM! Comprehensive scroll handle bounds testing with proper assertions.The tests cover key scenarios including document.body, scrollable elements, minimum height enforcement, and scrollTop handling. The past review concern about conditional expects has been properly addressed—the current implementation always asserts that
bounds.heightis a valid number before checking the minimum threshold.
108-151: LGTM! getVisibleBounds tests cover main scenarios and edge cases.The tests appropriately verify viewport bounds for document.body and regular elements, including edge cases like elements positioned outside the viewport. The implicit verification of the -2 subtraction (lines 145-150) is reasonable given the complex calculations involved.
174-188: LGTM! Caching behavior test confirms optimization works correctly.The test appropriately verifies that the zero element is created once and reused across multiple
getBoundscalls, confirming the caching mechanism functions as intended.
Summary by CodeRabbit
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.