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

Add concurrency to image creation #404

Merged
merged 11 commits into from
Mar 8, 2021

Conversation

shannonrothe
Copy link
Contributor

Noticed the comment so utilised Promise.all() to add concurrency to image creation 👍🏼

@sindresorhus
Copy link
Owner

I don't think Puppeteer can handle a load like this. I think you need to use https:/sindresorhus/p-all with a concurrency of maybe "number of CPUs * 2". Needs to be tested what gives the output performance.

@shannonrothe
Copy link
Contributor Author

@sindresorhus no problem, I might look into this a bit more when I get a chance 👍

Base automatically changed from master to main January 24, 2021 05:49
@sindresorhus
Copy link
Owner

Friendly bump :)

@shannonrothe
Copy link
Contributor Author

shannonrothe commented Mar 7, 2021

@sindresorhus still failing tests, not sure what the issue is with p-all 😕 Please check my changes and let me know how we can best approach 👍🏼

@sindresorhus
Copy link
Owner

You are using p-all incorrectly, which the error also points out.

@sindresorhus
Copy link
Owner

It might be better to use p-map since you're mapping this.sizes.

@shannonrothe
Copy link
Contributor Author

It might be better to use p-map since you're mapping this.sizes.

Done, should be okay now 👌🏼

source/index.ts Outdated
// TODO: Make this concurrent
this.items.push(await this.create(source.url, size, options));
}
this.items.push(...await pMap(
Copy link
Owner

Choose a reason for hiding this comment

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

This is not very readable. Make it a variable and then in a new statement, you can use destructuring instead of "push".

.gitignore Outdated Show resolved Hide resolved
@sindresorhus sindresorhus merged commit 42a265e into sindresorhus:main Mar 8, 2021
@sindresorhus
Copy link
Owner

Thanks :)

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