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(npm): use an http client with connection pool #16705

Merged
merged 1 commit into from Nov 18, 2022

Conversation

dsherret
Copy link
Member

Should make downloading npm packages faster and more reliable.

result_headers.insert(key_str, values_str);
}

if response.status().is_redirection() {
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering do we need a check here for a redirect loop, i.e. throw an error if the "Location" is same as current? can raise a separate issue if so. a limit on depth of redirects would be good also. maybe this is handled elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems there is a redirect limit at a higher level in file_fetcher.rs. I'm not super familiar with this code... this diff is pretty terrible for some reason, but all I did was indent the fetch_once function into a method.

Copy link
Contributor

@billywhizz billywhizz left a comment

Choose a reason for hiding this comment

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

LGTM but should probably seek a review from someone who knows the codebase better.

@dsherret dsherret merged commit 763d492 into denoland:main Nov 18, 2022
@dsherret dsherret deleted the npm_share_http_client branch November 18, 2022 22:28
bartlomieju pushed a commit that referenced this pull request Nov 24, 2022
Should make downloading npm packages faster and more reliable.
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.

None yet

3 participants