-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: npm compat on installing redirecting tarballs #10197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
💖 Thanks for opening this pull request! 💖 |
| normalizedBareSpecifier: wantedDependency.bareSpecifier, | ||
| resolution: { | ||
| tarball: resolvedUrl, | ||
| tarball: wantedDependency.bareSpecifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't change the URL that gets saved to the lockfile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure? this is how NPM does it: npm i https://yolkbot.xyz is
{
"name": "install-test",
"lockfileVersion": 3,
"requires": true,
"packages": {
"": {
"dependencies": {
"yolkbot": "https://yolkbot.xyz"
}
},
"node_modules/yolkbot": {
"version": "1.4.2",
"resolved": "https://yolkbot.xyz",
"integrity": "sha512-PIPs0+QaGu0eKrFPCuPM8eGKTmng3I6HFLwjvvFa5UjTTAO3TbkG48fGCbc/g2Fel/p56cKC3ETDxPwAuJlq5w==",
"dependencies": {}
}
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of the lockfile is to store the "resolved" endpoint. If the URL redirects, the final endpoint is the resolved one. Similarly pnpm stores the resolved commit hash for git-hosted dependencies, when installation is done by a branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the original URL can change the destination to a new tarball version, npm will actually break because the URL in the lockfile will not point to the same content anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have seen no issues with that in my use of npm and this method for months while i have published countless new versions.
to clarify, since i'm unfamiliar with how pnpm works, if the source at yolkbot.xyz updates despite the lockfile having the file tar file running pnpm i https://yolkbot.xyz will resolve to the new version? or the old one due to cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pnpm i https://yolkbot.xyz will update it
|
the issue we had been discussing should be fixed. tarball looks like this: lockfileVersion: '9.0'
settings:
autoInstallPeers: true
excludeLinksFromLockfile: false
importers:
.:
dependencies:
yolkbot:
specifier: https://yolkbot.xyz
version: https://yolkbot.xyz
packages:
[email protected]:
resolution: {integrity: sha512-QxeSGRej6FJPp4lVGMxCkJ4FtEXRITVaamU7UA7M2b12lt0XLrMav69eUs3hYJwPI1ZYeau8ea2kwI97oOghiA==}
yolkbot@https://yolkbot.xyz:
resolution: {tarball: https://registry.npmjs.org/yolkbot/-/yolkbot-1.4.2.tgz}
version: 1.4.2
snapshots:
[email protected]: {}
yolkbot@https://yolkbot.xyz:
dependencies:
wwws: 1.0.7 |
|
I think we can make an exception when the redirect is only changing the http protocol from http to https keeping the rest of the URL the same. In this case it should be fine to use the final URL. |
This is different compared to NPM and would result in a different package.json when running the same command with either prefix. For comparison, here is a package-lock.json of the same item: {
"name": "install-test",
"lockfileVersion": 3,
"requires": true,
"packages": {
"": {
"dependencies": {
"yolkbot": "http://yolkbot.xyz"
}
},
"node_modules/wwws": {
"version": "1.0.7",
"resolved": "https://registry.npmjs.org/wwws/-/wwws-1.0.7.tgz",
"integrity": "sha512-QxeSGRej6FJPp4lVGMxCkJ4FtEXRITVaamU7UA7M2b12lt0XLrMav69eUs3hYJwPI1ZYeau8ea2kwI97oOghiA=="
},
"node_modules/yolkbot": {
"version": "1.4.2",
"resolved": "http://yolkbot.xyz",
"integrity": "sha512-PIPs0+QaGu0eKrFPCuPM8eGKTmng3I6HFLwjvvFa5UjTTAO3TbkG48fGCbc/g2Fel/p56cKC3ETDxPwAuJlq5w==",
"dependencies": {
"wwws": "^1.0.7"
}
}
}
} |
|
cc @btea and @stainless-em |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes npm compatibility for handling redirecting tarballs by ensuring package resolution IDs use the original requested URL rather than the redirected URL. This aligns pnpm's behavior with npm and bun when dealing with tarball dependencies that redirect (e.g., from a registry URL to a CDN).
Key Changes:
- Modified the tarball resolver to use the original
bareSpecifierURL for package IDs and normalized specifiers, while still using the redirected URL for the actual tarball download location - Updated test cases to use
https://instead ofhttp://for registry URLs, reflecting current registry behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| resolving/tarball-resolver/src/index.ts | Changed to use wantedDependency.bareSpecifier for id and normalizedBareSpecifier fields instead of the redirected resolvedUrl, ensuring stable package identifiers |
| resolving/tarball-resolver/test/index.ts | Updated test URLs from http:// to https:// for npm registry and JSR registry test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
close #9802 --------- Co-authored-by: Zoltan Kochan <[email protected]>
closes #9802
this makes the behavior consistent with npm and bun (more info in #9802)
Related changes: