-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(gatsby-plugin-sharp): fix progressbar & caching layer #19717
fix(gatsby-plugin-sharp): fix progressbar & caching layer #19717
Conversation
I made it do less than before as the pre 2.30 version shows 80 and the new one is doing 74. It's not a bug, we process an input file twice which isn't happening with the new code as my check is much more early. |
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 think this is good! Thanks for adding test for it. Left few comments but feel free to merge
let finishedPromise = Promise.resolve() | ||
|
||
// Check if the output file already exists so we don't redo work. | ||
if (cachedOutputFiles.has(outputFile)) { | ||
finishedPromise = cachedOutputFiles.get(outputFile) |
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.
Should we return early here to avoid && !cachedOutputFiles.has(outputFile)
check in L116
Description
Fixes order of
reporter
params to fix progressbar. Also added caching layer for ouputFile back. If an outputPath is already scheduled we return the same promise, this was overlooked by me in the worker fix. It doesn't show in simple examples.Test version:
npm install --save gatsby-plugin-sharp@sharp-cache
Before the scheduleJob api was the same as we have in this PR.
gatsby/packages/gatsby-plugin-sharp/src/scheduler.js
Lines 54 to 60 in f9933b1
Pre 2.3.0:
Before:
After:
Related Issues
#19591 (comment)