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 4 commits into
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.37% 98.66% +0.29%
==========================================
Files 12 12
Lines 676 674 -2
Branches 176 175 -1
==========================================
Hits 665 665
+ Misses 11 9 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdates TypeScript configuration to include Node.js types; README and tests adjust npm provider root documentation and usage; npm provider code normalizes root, resolves runtime default via process.cwd(), refactors URL transforms, and updates cache key format. ChangesDocumentation
Google Provider
NPM Provider Implementation
NPM Provider Tests
TypeScript Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
🧹 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
|
@danielroe when you have time, I'd like your review on this! |
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
Improvements
Tests