fix(npm): improves root handling and resolve to local font files when possible#367
fix(npm): improves root handling and resolve to local font files when possible#367florian-lefebvre wants to merge 1 commit intomainfrom
Conversation
| const npmFetch = $fetch.create({ baseURL: cdn }) | ||
| const readFile = providerOptions.readFile | ||
| const root = providerOptions.root || '.' | ||
| const root = stripTrailingSlash(providerOptions.root || await import('node:process').then(m => m.cwd())) |
There was a problem hiding this comment.
An eslint rules requires us to use the module instead of the global. I decided to use a dynamic import to use it as a last resort, eg. in browser envs (although I doubt this is the primary usecase)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
==========================================
+ Coverage 98.48% 98.78% +0.30%
==========================================
Files 12 12
Lines 658 656 -2
Branches 171 170 -1
==========================================
Hits 648 648
+ Misses 10 8 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis PR updates the npm provider to resolve the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
src/providers/npm.ts (2)
316-316: Cache key format change will orphan existing entries.Adding
.jsonto the cache key is a breaking change for cached data. Existing cache entries without the.jsonsuffix will become unreachable and remain orphaned until they expire (one week). This isn't a functional issue—just be aware that users may see cache misses after upgrading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/npm.ts` at line 316, The cache key was changed to include a ".json" suffix in the const key = `npm:${pkgName}/${cssFile}-${hash(options)}.json`, which will orphan existing entries; revert to the previous key format or implement backward-compatibility in the cache read path by attempting the old key (e.g., const oldKey = `npm:${pkgName}/${cssFile}-${hash(options)}`) when fetching and, if found, either migrate it to the new key or use it directly, and ensure any cache write still uses the chosen canonical key so subsequent reads are consistent (update the code around the key variable usage to try oldKey before new key and migrate if necessary).
132-137: Consider handling backslashes for Windows compatibility.The
stripTrailingSlash()function only handles forward slashes. On Windows, paths might use backslashes. Consider also stripping trailing backslashes.♻️ Optional fix for Windows compatibility
function stripTrailingSlash(str: string): string { - if (str.endsWith('/')) { - return str.slice(0, -1) + if (str.endsWith('/') || str.endsWith('\\')) { + return str.slice(0, -1) } return str }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/providers/npm.ts` around lines 132 - 137, stripTrailingSlash currently only trims a trailing forward slash; update the function (stripTrailingSlash) to also handle Windows backslashes by trimming any trailing forward or backslash characters (e.g., treat both '/' and '\' as trailing separators) so paths like "C:\path\" are normalized; implement by removing trailing slash/backslash characters (use a single logic that strips both types) and return the cleaned string from stripTrailingSlash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/providers/npm.ts`:
- Line 316: The cache key was changed to include a ".json" suffix in the const
key = `npm:${pkgName}/${cssFile}-${hash(options)}.json`, which will orphan
existing entries; revert to the previous key format or implement
backward-compatibility in the cache read path by attempting the old key (e.g.,
const oldKey = `npm:${pkgName}/${cssFile}-${hash(options)}`) when fetching and,
if found, either migrate it to the new key or use it directly, and ensure any
cache write still uses the chosen canonical key so subsequent reads are
consistent (update the code around the key variable usage to try oldKey before
new key and migrate if necessary).
- Around line 132-137: stripTrailingSlash currently only trims a trailing
forward slash; update the function (stripTrailingSlash) to also handle Windows
backslashes by trimming any trailing forward or backslash characters (e.g.,
treat both '/' and '\' as trailing separators) so paths like "C:\path\" are
normalized; implement by removing trailing slash/backslash characters (use a
single logic that strips both types) and return the cleaned string from
stripTrailingSlash.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35712c94-45eb-4bf1-9a4e-d1490dd94853
📒 Files selected for processing (5)
README.mdsrc/providers/google.tssrc/providers/npm.tstest/providers/npm.test.tstsconfig.json
1 participant
Reported at withastro/astro#16218.
While digging the reproduction, I uncovered a few bugs:
.json, making it harder to debugrootto'.'causes a bunch of issues when manipulating paths. I updated it toprocess.cwd()which does the same thing. I updated tests to set it back to'.'to make it easier to mockSummary by CodeRabbit
Bug Fixes
'.'value.Code Quality