fix: don't lock ephemeral tarball urls#9532
Conversation
|
💖 Thanks for opening this pull request! 💖 |
There was a problem hiding this comment.
Pull Request Overview
This PR refines the tarball resolution logic so that only immutable tarball URLs (as determined by the cache‐control header) are locked, addressing issue #9531.
- Update test cases to explicitly differentiate between immutable and mutable tarball URLs.
- Adjust the resolveFromTarball function to conditionally lock redirects based on the immutability of the response.
- Update the changeset with relevant messaging.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| resolving/tarball-resolver/test/index.ts | Renamed tests and added a new test to cover npm.jsr.io registry behavior. |
| resolving/tarball-resolver/src/index.ts | Modified the tarball resolution logic to check for immutability before locking. |
| .changeset/bright-oranges-like.md | Updated changeset to document the new behavior regarding URL locking. |
Comments suppressed due to low confidence (1)
resolving/tarball-resolver/test/index.ts:21
- [nitpick] Consider adding a corresponding test case for a mutable tarball URL from npm.jsr.io to ensure both branches of the conditional logic are fully covered.
test('tarball from npm.jsr.io registry (immutable)', async () => {
|
|
||
| // If there are redirects, we want to get the final URL address | ||
| const { url: resolvedUrl } = await fetchFromRegistry(wantedDependency.bareSpecifier, { method: 'HEAD' }) | ||
| let resolvedUrl |
There was a problem hiding this comment.
[nitpick] Consider adding an explicit type annotation (e.g., let resolvedUrl: string) for improved clarity and type safety.
| let resolvedUrl | |
| let resolvedUrl: string |
| }) | ||
|
|
||
| test('tarball not from npm registry', async () => { | ||
| test('tarball not from npm registry (mutable)', async () => { |
There was a problem hiding this comment.
Verify that the expected URL for mutable tarballs is correct, as the logic now preserves the original bareSpecifier when the response isn't immutable.
|
Congrats on merging your first pull request! 🎉🎉🎉 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Fixes #9531 by only locking redirects when the final target is immutable