feat: route pkg@version from deps, direct nav, and search for any valid specifier#1723
feat: route pkg@version from deps, direct nav, and search for any valid specifier#1723
pkg@version from deps, direct nav, and search for any valid specifier#1723Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
serhalp
changed the title
pkg@version from deps, direct nav, and search for any valid specifierpkg@version from deps, direct nav, and search for any valid specifier
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Awesome idea, I really like this one. Two birds with one stone 🚀 |
📝 WalkthroughWalkthroughIntroduces parsing and handling for package specifiers of the form "pkg@version": a new shared utility Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 (3)
shared/utils/parse-package-param.ts (1)
54-61: Trailing@edge case may produce unexpected results.When the input ends with
@but has no version (e.g.,'esbuild@'), the function returns{ name: 'esbuild@' }because theversionslice is empty. This retains the trailing@in the package name, which is not a valid npm package name.Consider whether this should instead return
{ name: 'esbuild' }to strip the trailing@:💡 Suggested fix
export function parsePackageSpecifier(input: string): { name: string; version?: string } { const atIndex = input.startsWith('@') ? input.indexOf('@', 1) : input.indexOf('@') if (atIndex > 0) { const version = input.slice(atIndex + 1) if (version) return { name: input.slice(0, atIndex), version } + // Trailing @ with no version - return name without the @ + return { name: input.slice(0, atIndex) } } return { name: input } }modules/runtime/server/cache.ts (1)
222-238: Consider usingparsePackageSpecifierto reduce code duplication.This function contains inline parsing logic for extracting the package name and version specifier (lines 226-238) that duplicates the logic in
parsePackageSpecifier. The same pattern appears inprocessSingleVersionsMeta(lines 300-312).Since
parsePackageSpecifierwas introduced to centralise this parsing, consider refactoring these functions to use it for consistency and maintainability.♻️ Suggested refactor
async function processSingleFastNpmMeta( packageQuery: string, storage: ReturnType<typeof useStorage>, metadata: boolean, ): Promise<Record<string, unknown>> { - let packageName = packageQuery - let specifier = 'latest' - - if (packageName.startsWith('@')) { - const atIndex = packageName.indexOf('@', 1) - if (atIndex !== -1) { - specifier = packageName.slice(atIndex + 1) - packageName = packageName.slice(0, atIndex) - } - } else { - const atIndex = packageName.indexOf('@') - if (atIndex !== -1) { - specifier = packageName.slice(atIndex + 1) - packageName = packageName.slice(0, atIndex) - } - } + const { name: packageName, version } = parsePackageSpecifier(packageQuery) + const specifier = version ?? 'latest'test/unit/app/utils/router.spec.ts (1)
100-107: Consider adding a test case forundefinedversion.The test covers
nullversion but notundefined. Since the function signature acceptsstring | null | undefined, it would be good to explicitly test theundefinedcase for completeness.💡 Suggested addition
describe('with null/undefined version', () => { it('returns package route for null version', () => { expect(packageRoute('react', null)).toEqual({ name: 'package', params: { org: '', name: 'react' }, }) }) + + it('returns package route for undefined version', () => { + expect(packageRoute('react', undefined)).toEqual({ + name: 'package', + params: { org: '', name: 'react' }, + }) + }) })
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/Package/Versions.vueapp/pages/search.vueapp/utils/router.tsmodules/runtime/server/cache.tsshared/utils/parse-package-param.tstest/e2e/search-at-version.spec.tstest/unit/app/utils/router.spec.tstest/unit/shared/utils/parse-package-param.spec.ts
|
I like that! ❤️ I just checked it out locally and jumping to the correct version by pressing enter is amazing. The problem I see is I do not know about that functionality if I don't press enter and the search results do not indicate that this is even possible (because the package is not shown). Maybe a combination with stripping the version for the search as in #1626 could help so that the correct package will also be shown in the results? |
Hi! Sorry if this is a bit out of context 😅 |
|
…nstead of 404 Dependency links with version ranges (e.g. "^18.0.0 || ^19.0.0", ">15 <=16.0.2") previously navigated to a non-existent version page and 404'd. Now `packageRoute()` distinguishes exact versions from ranges: exact versions link to the version page, while ranges link to the package page with `?semver=<range>#versions`, pre-populating the existing "Filter by semver" input. Closes #1120
Searching for `esbuild@0.25.12` or `@angular/core@^18` and pressing Enter now navigates directly to the package version page (or semver filter for ranges) instead of returning no results. Since in the previous commit I added support for linking to the package page with the a pre-filled semver version specifier in the query string to populate the version filter, this supports both exact versions and semver ranges in the search input. Closes #1416
serhalp
force-pushed
the
serhalp/fix-version-sorting
branch
from
56fa96e to
dffa4f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Package/Versions.vue (1)
108-109: Minor: Redundant|| ''fallback.The ternary expression already returns
''for non-string cases, making the trailing|| ''unnecessary.✨ Suggested simplification
-const semverFilter = ref((typeof route.query.semver === 'string' ? route.query.semver : '') || '') +const semverFilter = ref(typeof route.query.semver === 'string' ? route.query.semver : '')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2800da71-7154-45bc-a870-b32110a5fec8
📒 Files selected for processing (4)
app/components/Package/Versions.vueapp/pages/search.vueapp/utils/router.tsdocs/content/2.guide/1.features.md
✅ Files skipped from review due to trivial changes (2)
- app/pages/search.vue
- docs/content/2.guide/1.features.md
| // Semver range filter (initialized from ?semver= query param if present) | ||
| const semverFilter = ref((typeof route.query.semver === 'string' ? route.query.semver : '') || '') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any watchers or other mechanisms that sync route.query.semver to semverFilter
rg -n 'route\.query\.semver' app/components/Package/Versions.vueRepository: npmx-dev/npmx.dev
Length of output: 163
🏁 Script executed:
cat -n app/components/Package/Versions.vue | head -150Repository: npmx-dev/npmx.dev
Length of output: 5561
🏁 Script executed:
rg -n 'watch\(|onBeforeRouteUpdate' app/components/Package/Versions.vueRepository: npmx-dev/npmx.dev
Length of output: 188
🏁 Script executed:
rg -n 'Versions\.vue\|<Versions' app/pages/ -A 3Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
rg -n ':key=' app/pages/package/\[\[org\]\]/\[name\]\.vue | head -20Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
rg -n 'import.*Versions' app/ -l | grep -v node_modulesRepository: npmx-dev/npmx.dev
Length of output: 344
🏁 Script executed:
rg -n '<Versions' app/ -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 587
🏁 Script executed:
fd -name '\[name\].vue' app/pages/package/Repository: npmx-dev/npmx.dev
Length of output: 291
🏁 Script executed:
find app/pages/package -name '[name].vue' -o -name 'index.vue'Repository: npmx-dev/npmx.dev
Length of output: 103
🏁 Script executed:
cat -n app/pages/package/\[\[org\]\]/\[name\].vue | head -200Repository: npmx-dev/npmx.dev
Length of output: 6760
🏁 Script executed:
rg -n '<Versions' app/pages/package/\[\[org\]\]/\[name\]/index.vue -B 2 -A 5Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
wc -l app/pages/package/\[\[org\]\]/\[name\]/index.vueRepository: npmx-dev/npmx.dev
Length of output: 106
🏁 Script executed:
cat app/pages/package/\[\[org\]\]/\[name\]/index.vueRepository: npmx-dev/npmx.dev
Length of output: 221
🏁 Script executed:
find app/pages/package -type f -name '*.vue' | xargs wc -l | sort -rnRepository: npmx-dev/npmx.dev
Length of output: 271
🏁 Script executed:
ls -la app/pages/package/\[\[org\]\]/Repository: npmx-dev/npmx.dev
Length of output: 292
🏁 Script executed:
rg -n 'Versions' app/pages/package/\[\[org\]\]/\[name\].vue -B 3 -A 3Repository: npmx-dev/npmx.dev
Length of output: 2760
🏁 Script executed:
rg -n '<Versions' app/pages/package/\[\[org\]\]/\[name\].vue -B 5 -A 5Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
sed -n '964,980p' app/pages/package/\[\[org\]\]/\[name\].vueRepository: npmx-dev/npmx.dev
Length of output: 718
🏁 Script executed:
rg -n 'PackageVersions' app/pages/package/\[\[org\]\]/\[name\].vue -A 10Repository: npmx-dev/npmx.dev
Length of output: 1041
🏁 Script executed:
cat -n app/components/Package/Versions.vue | sed -n '100,350p'Repository: npmx-dev/npmx.dev
Length of output: 10420
🏁 Script executed:
cat -n app/components/Package/Versions.vue | sed -n '350,550p'Repository: npmx-dev/npmx.dev
Length of output: 8047
🏁 Script executed:
cat -n app/components/Package/Versions.vue | tail -50Repository: npmx-dev/npmx.dev
Length of output: 1819
🏁 Script executed:
rg -n 'onBeforeRouteUpdate\|watch.*route' app/components/Package/Versions.vueRepository: npmx-dev/npmx.dev
Length of output: 43
Watch route.query.semver to sync filter state on navigation.
The semverFilter is initialised once at component setup from route.query.semver. Without a watcher on the route parameter, internal navigation (e.g., from /package/foo?semver=^1 to /package/foo?semver=^2) won't update the filter because the component instance reuses without unmounting. Add a watcher on route.query.semver to keep the filter in sync with URL changes.
4 participants
🔗 Linked issue
Closes #1120
Closes #1416
🧭 Context
We have two seemingly separate issues:
^18.0.0 || ^19.0.0,>15 <=16.0.2) currently navigate to a non-existent version page and result in a 404.nuxt@4.0.1ornuxt@>=4ornuxt@>=4<6(convenient if pasting from somewhere) does not result in a match. There's a proposed temporary solution in feat: strip version info from the search query #1626 to strip the version segment from the query.📚 Description
https://npmx.dev/package/nuxt/v/:versionwill route to the specific versioned package page ifversionis a specific version string and route to the package page with?semver=<version>otherwise.nuxt@4.0.1ornuxt@>=4ornuxt@>=4<6and pressing Enter now navigates directly to the package version page for a specific version or to the semver-filtered package page for a semver specifier, instead of returning no results. This just works for both exact versions and semver ranges, because of the above routing change.npmx.smart.pkg@v.routing.mp4