-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
perf: improve tarball fetching speed #6819
Conversation
27e1818
to
1575bd2
Compare
@@ -158,27 +160,57 @@ export function createDownloader ( | |||
? throttle(opts.onProgress, 500) | |||
: undefined | |||
let downloaded = 0 | |||
const chunks: Buffer[] = [] | |||
res.body!.on('data', (chunk: Buffer) => { |
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.
Since you aren't wrapping any callback-based APIs other than the data
event here, you might find it convenient to take advantage of the fact that Node's Readable
interface implements [Symbol.asyncIterator]
:
// This will handle the 'data', 'error', and 'end' events.
for await (const chunk of res.body) {
chunks.push(chunk);
downloaded += chunk.length;
onProgress?.(downloaded);
}
if (size !== null && size !== downloaded) {
throw new BadTarballError(...);
}
// etc.
@@ -130,7 +130,7 @@ test('fail when integrity check fails two times in a row', async () => { | |||
new TarballIntegrityError({ | |||
algorithm: 'sha512', | |||
expected: 'sha1-HssnaJydJVE+rbyZFKc/VAi+enY=', | |||
found: 'sha512-VuFL1iPaIxJK/k3gTxStIkc6+wSiDwlLdnCWNZyapsVLobu/0onvGOZolASZpfBFiDJYrOIGiDzgLIULTW61Vg== sha1-ACjKMFA7S6uRFXSDFfH4aT+4B4Y=', | |||
found: 'sha1-ACjKMFA7S6uRFXSDFfH4aT+4B4Y=', |
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 am not sure why this test had to be changed but I guess it is OK. The error message is even better this way.
Related comment: https://github.com/orgs/pnpm/discussions/6376#discussioncomment-6420573