Skip to content

feat: unifying npm registry requests with caching - #641

Merged
danielroe merged 18 commits into
npmx-dev:mainfrom
OrbisK:feat/concept-npm-requests
Feb 7, 2026
Merged

feat: unifying npm registry requests with caching#641
danielroe merged 18 commits into
npmx-dev:mainfrom
OrbisK:feat/concept-npm-requests

Conversation

@OrbisK

Copy link
Copy Markdown
Contributor

I think it is not an easy change to refactor all npm requests to useNpmRegistry (yet). As a first step, we could migrate all npm requests to $npmRegistry and $npmApi. Wdyt?

@vercel

vercelBot commented Feb 1, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

ProjectDeploymentActionsUpdated (UTC)
npmx.devReadyReadyPreview, CommentFeb 7, 2026 8:37pm
2 Skipped Deployments
ProjectDeploymentActionsUpdated (UTC)
docs.npmx.devIgnoredIgnoredPreviewFeb 7, 2026 8:37pm
npmx-lunariaIgnoredIgnoredFeb 7, 2026 8:37pm

Request Review

…ests
# Conflicts:
#	app/composables/useNpmRegistry.ts
#	app/utils/package-name.ts
@OrbisKOrbisK changed the title wip: proof of concept unifying all registry requestsfeat: unifying npm registry requests with cachingFeb 1, 2026
@danielroe

Copy link
Copy Markdown
Member

this looks good!

would you investigate why the browser tests might be failing?

@OrbisK

Copy link
Copy Markdown
ContributorAuthor

this looks good!

would you investigate why the browser tests might be failing?

Working on it. Maybe you know what the issue is. e.g. the org page is broken.

https://npmxdev-git-fork-orbisk-feat-concept-npm-requests-poetry.vercel.app/@nuxt

image

Here: https://github.com/npmx-dev/npmx.dev/pull/641/changes#diff-93fe589cec6bc2b865b353a91ba90b4407706ff4baf16189a4c2c538351aa26aR523

@OrbisK

OrbisK commented Feb 1, 2026

Copy link
Copy Markdown
ContributorAuthor

I think the problem is that cachedFetch from the plugin uses the server side fetch on the client 🤷🏽‍♂️

I think I have a fix.

@OrbisK

Copy link
Copy Markdown
ContributorAuthor

I am super confused. Are $npmRegistry request meant to run client side? Because there are cors issues when requesting the orgs

@OrbisK

Copy link
Copy Markdown
ContributorAuthor

Okay. The request returns the data and 200 but cors is still blocking it 🤔

@danielroe

danielroe commented Feb 1, 2026

Copy link
Copy Markdown
Member

$npmRegistry should be idempotent, yes. (it should be safe to call in either case)

if there's stale data (or missing data) from the server, then it will refetch on the client

@danielroe
danielroe self-requested a review February 1, 2026 17:24
@OrbisK

Copy link
Copy Markdown
ContributorAuthor

Progress: I have fixed it locally. But i am not sure how...

@vercel

vercelBot commented Feb 7, 2026

Copy link
Copy Markdown
Contributor

@danielroe is attempting to deploy a commit to the serhalp's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
app/composables/npm/useNpmSearch.ts (1)

53-215: Consider splitting useNpmSearch into smaller helpers.

It now covers initial search, single-character lookup, paging, caching, and watchers; breaking it up would improve readability and future changes. As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".

Comment on lines 56 to 62
batch.map(async name => {
const encoded = encodePackageName(name)
const data = await $fetch<{ downloads: number }>(
`${NPM_API}/downloads/point/last-week/${encoded}`,
const { data } = await $npmApi<{ downloads: number }>(
`/downloads/point/last-week/${encoded}`,
)
return { name, downloads: data.downloads }
}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Missing options parameter for scoped package downloads.

The scoped package individual fetches do not receive the options parameter (which includes signal for request cancellation). This is inconsistent with the unscoped bulk fetch on line 34, which correctly passes options. If the parent request is aborted, these requests will continue executing.

🔧 Proposed fix to pass options
 batch.map(async name => {
const encoded = encodePackageName(name)
const { data } = await $npmApi<{ downloads: number }>(
`/downloads/point/last-week/${encoded}`,
+ options,
)
return { name, downloads: data.downloads }
}),
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
batch.map(asyncname=>{
constencoded=encodePackageName(name)
constdata=await$fetch<{downloads: number}>(
`${NPM_API}/downloads/point/last-week/${encoded}`,
const{ data }=await$npmApi<{downloads: number}>(
`/downloads/point/last-week/${encoded}`,
)
return{ name,downloads: data.downloads}
}),
batch.map(asyncname=>{
constencoded=encodePackageName(name)
const{ data }=await$npmApi<{downloads: number}>(
`/downloads/point/last-week/${encoded}`,
options,
)
return{ name,downloads: data.downloads}
}),

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@OrbisK@danielroe@43081j@serhalp