Skip to content
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

fix: wait for size event from body for contentLength #366

Closed
wants to merge 1 commit into from

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented May 6, 2024

By default the Accept-Encoding request header is set to gzip,deflate which makes it so we don't get back a Content-Length response header from the registry. But npm-registry-fetch sets up a size event on the response body that we can wait for before continuing if we don't get a Content-Length header.

This fixes the content length issue in #7463 for me when testing locally, but I need to run some more CLI tests with this linked to see if there are any adverse effects.

@lukekarrys lukekarrys requested a review from a team as a code owner May 6, 2024 07:11
@@ -96,7 +98,11 @@ class RegistryFetcher extends Fetcher {
integrity: null,
})
const packument = await res.json()
packument._contentLength = +res.headers.get('content-length')
let contentLength = res.headers.get('content-length') ?? null
Copy link
Member

Choose a reason for hiding this comment

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

Is this ?? effectively the same test as isStream? Do we really need them both?

wraithgar
wraithgar previously approved these changes May 6, 2024
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

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

One take-it-or-leave-it question.

@wraithgar wraithgar self-requested a review May 6, 2024 16:35
@wraithgar wraithgar dismissed their stale review May 6, 2024 16:36

not sure about blocking

lukekarrys added a commit that referenced this pull request May 6, 2024
The opposite of #366. This is not a change in behavior but makes it clear that we don't expect _contentLength to be set when the request is coming directly from the registry when there is no cache.
lukekarrys added a commit that referenced this pull request May 6, 2024
The opposite of #366. This is not a change in behavior but makes it
clear that we don't expect _contentLength to be set when the request is
coming directly from the registry when there is no cache.
@lukekarrys
Copy link
Contributor Author

Closing in favor of #368. We don't want to wait for the entire stream to be decompressed and get the size event before moving on here. We could potentially await the even further down the line, but that would get pretty complex as we would need to wait for the contentLength before caching the result.

Instead #368 makes it clear we won't get a contentLength from the registry, so we won't cache those in-memory since we can't be sure of their size.

@lukekarrys lukekarrys closed this May 6, 2024
Sign up for free to 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.

2 participants