-
Notifications
You must be signed in to change notification settings - Fork 52
Use workers to process files in parallel #280
Conversation
|
Walker Burgin seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/[email protected] |
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat's wrong with native code?Contains native code which could be a vector to obscure malicious code, and generally decrease the likelihood of reproducible or reliable installs. Ensure that native code bindings are expected. Consumers may consider pure JS and functionally similar alternatives to avoid the challenges and risks associated with native code bindings. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
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.
Thank you!
* Bump @types/node to 20.x.x and increase 'engines' check * Add dependency on piscina * Add a worker pool --------- Co-authored-by: Walker Burgin <[email protected]>
This PR adds a worker pool to process files in parallel.
Notes
The
piscina
typings referenceEventEmitterAsyncResource
, which was added in Node 16.14.0 but (apparently) wasn't exposed by@types/node
until20.x.x
. I bumped@types/node
and increased theengines
check to16.x
, which feels reasonable given that Node 18 is already in maintenance and the current LTS is 20.x.x (docs). Not sure if you're willing to take that change or not.Results
Here's the command I'm running to compare performance. We're currently using SWC for minification at the last stage of a build pipeline, so this test primarily reflects that use case. The
fixtures
directory here contains ~2400 JavaScript files.--sync
(before this PR)--sync
(before this PR)--workers
(after this PR)As a sidenote, I was a little surprised that this is still meaningfully slower than replacing
transformFile()
withminify()
(better emulating the custom code we're using internally at the moment). I'm not really familiar with SWCs implementation, so I'm not sure whether I should be surprised by this or not.transformFile()
withminify()
and--sync
transformFile()
withminify()
without--sync
transformFile()
withminify()
and add--workers
The minification docs imply that it's appropriate to run the CLI for minification like this. Maybe a dedicated
minify
command might make sense?Fixes #274