fix(gatsby-plugin-sharp): hot fix for "Generating image thumbnails" progress bar reporting duplicates that don't do actual processing (#20839)

pieh · wardpeet · commit db36d6995659 · 2020-01-27T12:25:02.000+01:00
diff --git a/packages/gatsby-plugin-sharp/src/__tests__/__snapshots__/index.js.snap b/packages/gatsby-plugin-sharp/src/__tests__/__snapshots__/index.js.snap
@@ -904,6 +904,7 @@ exports[`gatsby-plugin-sharp queueImageResizing with createJob file name works w
         "outputDir": "<PROJECT_ROOT>/public/static/1234",
       },
       Object {},
+      undefined,
     ],
   ],
   "results": Array [
@@ -945,6 +946,7 @@ exports[`gatsby-plugin-sharp queueImageResizing with createJob should round heig
         "outputDir": "<PROJECT_ROOT>/public/static/1234",
       },
       Object {},
+      undefined,
     ],
   ],
   "results": Array [
diff --git a/packages/gatsby-plugin-sharp/src/__tests__/utils.js b/packages/gatsby-plugin-sharp/src/__tests__/utils.js
@@ -1,23 +1,28 @@
 jest.mock(`gatsby-cli/lib/reporter`)
 jest.mock(`progress`)
-const { createProgress } = require(`../utils`)
+const {
+  createGatsbyProgressOrFallbackToExternalProgressBar,
+} = require(`../utils`)
 const reporter = require(`gatsby-cli/lib/reporter`)
 const progress = require(`progress`)
 
-describe(`createProgress`, () => {
+describe(`createGatsbyProgressOrFallbackToExternalProgressBar`, () => {
   beforeEach(() => {
     progress.mockClear()
   })
 
   it(`should use createProgress from gatsby-cli when available`, () => {
-    createProgress(`test`, reporter)
+    createGatsbyProgressOrFallbackToExternalProgressBar(`test`, reporter)
     expect(reporter.createProgress).toBeCalled()
     expect(progress).not.toBeCalled()
   })
 
   it(`should fallback to a local implementation when createProgress does not exists on reporter`, () => {
     reporter.createProgress = null
-    const bar = createProgress(`test`, reporter)
+    const bar = createGatsbyProgressOrFallbackToExternalProgressBar(
+      `test`,
+      reporter
+    )
     expect(progress).toHaveBeenCalledTimes(1)
     expect(bar).toHaveProperty(`start`, expect.any(Function))
     expect(bar).toHaveProperty(`tick`, expect.any(Function))
@@ -26,7 +31,7 @@ describe(`createProgress`, () => {
   })
 
   it(`should fallback to a local implementation when no reporter is present`, () => {
-    const bar = createProgress(`test`)
+    const bar = createGatsbyProgressOrFallbackToExternalProgressBar(`test`)
     expect(progress).toHaveBeenCalledTimes(1)
     expect(bar).toHaveProperty(`start`, expect.any(Function))
     expect(bar).toHaveProperty(`tick`, expect.any(Function))
diff --git a/packages/gatsby-plugin-sharp/src/gatsby-node.js b/packages/gatsby-plugin-sharp/src/gatsby-node.js
@@ -1,9 +1,9 @@
 const {
   setBoundActionCreators,
-  getProgressBar,
   // queue: jobQueue,
   // reportError,
 } = require(`./index`)
+const { getProgressBar, createOrGetProgressBar } = require(`./utils`)
 
 const { setPluginOptions } = require(`./plugin-options`)
 
@@ -63,9 +63,47 @@ const finishProgressBar = () => {
 exports.onPostBuild = () => finishProgressBar()
 exports.onCreateDevServer = () => finishProgressBar()
 
-exports.onPreBootstrap = ({ actions }, pluginOptions) => {
+exports.onPreBootstrap = ({ actions, emitter, reporter }, pluginOptions) => {
   setBoundActionCreators(actions)
   setPluginOptions(pluginOptions)
+
+  // below is a hack / hot fix for confusing progress bar behaviour
+  // that doesn't recognize duplicate jobs, as it's now
+  // in gatsby core internals (if `createJobV2` is available)
+  // we should remove this or make this code path not hit
+  // (we should never use emitter in plugins)
+  // as soon as possible (possibly by moving progress bar handling
+  // inside jobs-manager in core)
+
+  if (emitter) {
+    // track how many image transformation each job has
+    // END_JOB_V2 doesn't contain that information
+    // so we store it in <JobContentHash, TransformsCount> map
+    // when job is created. Then retrieve that when job finishes
+    // and remove that entry from the map.
+    const imageCountInJobsMap = new Map()
+
+    emitter.on(`CREATE_JOB_V2`, action => {
+      if (action.plugin.name === `gatsby-plugin-sharp`) {
+        const job = action.payload.job
+        const imageCount = job.args.operations.length
+        imageCountInJobsMap.set(job.contentDigest, imageCount)
+        const progress = createOrGetProgressBar(reporter)
+        progress.addImageToProcess(imageCount)
+      }
+    })
+
+    emitter.on(`END_JOB_V2`, action => {
+      if (action.plugin.name === `gatsby-plugin-sharp`) {
+        const jobContentDigest = action.payload.jobContentDigest
+        const imageCount = imageCountInJobsMap.get(jobContentDigest)
+        const progress = createOrGetProgressBar(reporter)
+        progress.tick(imageCount)
+        imageCountInJobsMap.delete(jobContentDigest)
+      }
+    })
+  }
+
   // normalizedOptions = setPluginOptions(pluginOptions)
 }
 
diff --git a/packages/gatsby-plugin-sharp/src/index.js b/packages/gatsby-plugin-sharp/src/index.js
@@ -18,7 +18,6 @@ const {
 const { memoizedTraceSVG, notMemoizedtraceSVG } = require(`./trace-svg`)
 const duotone = require(`./duotone`)
 const { IMAGE_PROCESSING_JOB_NAME } = require(`./gatsby-worker`)
-const { createProgress } = require(`./utils`)
 
 const imageSizeCache = new Map()
 const getImageSize = file => {
@@ -36,46 +35,6 @@ const getImageSize = file => {
   }
 }
 
-let progressBar
-let pendingImagesCounter = 0
-let firstPass = true
-const createOrGetProgressBar = reporter => {
-  if (!progressBar) {
-    progressBar = createProgress(`Generating image thumbnails`, reporter)
-
-    const originalDoneFn = progressBar.done
-
-    // TODO this logic should be moved to the reporter.
-    // when done is called we remove the progressbar instance and reset all the things
-    // this will be called onPostBuild or when devserver is created
-    progressBar.done = () => {
-      originalDoneFn.call(progressBar)
-      progressBar = null
-      pendingImagesCounter = 0
-    }
-
-    // when we create a progressBar for the second time so when .done() has been called before
-    // we create a modified tick function that automatically stops the progressbar when total is reached
-    // this is used for development as we're watching for changes
-    if (!firstPass) {
-      let progressBarCurrentValue = 0
-      const originalTickFn = progressBar.tick
-      progressBar.tick = (ticks = 1) => {
-        originalTickFn.call(progressBar, ticks)
-        progressBarCurrentValue += ticks
-
-        if (progressBarCurrentValue === pendingImagesCounter) {
-          progressBar.done()
-        }
-      }
-    }
-    firstPass = false
-  }
-
-  return progressBar
-}
-exports.getProgressBar = () => progressBar
-
 // Bound action creators should be set when passed to onPreInit in gatsby-node.
 // ** It is NOT safe to just directly require the gatsby module **.
 // There is no guarantee that the module resolved is the module executing!
@@ -148,16 +107,6 @@ function prepareQueue({ file, args }) {
 }
 
 function createJob(job, { reporter }) {
-  const progressBar = createOrGetProgressBar(reporter)
-
-  if (pendingImagesCounter === 0) {
-    progressBar.start()
-  }
-
-  const transformsCount = job.args.operations.length
-  pendingImagesCounter += transformsCount
-  progressBar.total = pendingImagesCounter
-
   // Jobs can be duplicates and usually are long running tasks.
   // Because of that we shouldn't use async/await and instead opt to use
   // .then() /.catch() handlers, because this allows V8 to release
@@ -169,16 +118,12 @@ function createJob(job, { reporter }) {
   if (boundActionCreators.createJobV2) {
     promise = boundActionCreators.createJobV2(job)
   } else {
-    promise = scheduleJob(job, boundActionCreators)
+    promise = scheduleJob(job, boundActionCreators, reporter)
   }
 
-  promise
-    .catch(err => {
-      reporter.panic(err)
-    })
-    .then(() => {
-      progressBar.tick(transformsCount)
-    })
+  promise.catch(err => {
+    reporter.panic(err)
+  })
 
   return promise
 }
diff --git a/packages/gatsby-plugin-sharp/src/scheduler.js b/packages/gatsby-plugin-sharp/src/scheduler.js
@@ -4,6 +4,7 @@ const fs = require(`fs-extra`)
 const got = require(`got`)
 const { createContentDigest } = require(`gatsby-core-utils`)
 const worker = require(`./gatsby-worker`)
+const { createOrGetProgressBar } = require(`./utils`)
 
 const processImages = async (jobId, job, boundActionCreators) => {
   try {
@@ -16,7 +17,7 @@ const processImages = async (jobId, job, boundActionCreators) => {
 }
 
 const jobsInFlight = new Map()
-const scheduleJob = async (job, boundActionCreators) => {
+const scheduleJob = async (job, boundActionCreators, reporter) => {
   const inputPaths = job.inputPaths.filter(
     inputPath => !fs.existsSync(path.join(job.outputDir, inputPath))
   )
@@ -70,12 +71,16 @@ const scheduleJob = async (job, boundActionCreators) => {
     { name: `gatsby-plugin-sharp` }
   )
 
+  const progressBar = createOrGetProgressBar(reporter)
+  const transformsCount = job.args.operations.length
+  progressBar.addImageToProcess(transformsCount)
+
   const promise = new Promise((resolve, reject) => {
     setImmediate(() => {
-      processImages(jobId, convertedJob, boundActionCreators).then(
-        resolve,
-        reject
-      )
+      processImages(jobId, convertedJob, boundActionCreators).then(result => {
+        progressBar.tick(transformsCount)
+        resolve(result)
+      }, reject)
     })
   })
 
diff --git a/packages/gatsby-plugin-sharp/src/utils.js b/packages/gatsby-plugin-sharp/src/utils.js
@@ -1,7 +1,10 @@
 const ProgressBar = require(`progress`)
 
 // TODO remove in V3
-export function createProgress(message, reporter) {
+function createGatsbyProgressOrFallbackToExternalProgressBar(
+  message,
+  reporter
+) {
   if (reporter && reporter.createProgress) {
     return reporter.createProgress(message)
   }
@@ -26,3 +29,57 @@ export function createProgress(message, reporter) {
     },
   }
 }
+
+let progressBar
+let pendingImagesCounter = 0
+let firstPass = true
+const createOrGetProgressBar = reporter => {
+  if (!progressBar) {
+    progressBar = createGatsbyProgressOrFallbackToExternalProgressBar(
+      `Generating image thumbnails`,
+      reporter
+    )
+
+    const originalDoneFn = progressBar.done
+
+    // TODO this logic should be moved to the reporter.
+    // when done is called we remove the progressbar instance and reset all the things
+    // this will be called onPostBuild or when devserver is created
+    progressBar.done = () => {
+      originalDoneFn.call(progressBar)
+      progressBar = null
+      pendingImagesCounter = 0
+    }
+
+    progressBar.addImageToProcess = imageCount => {
+      if (pendingImagesCounter === 0) {
+        progressBar.start()
+      }
+      pendingImagesCounter += imageCount
+      progressBar.total = pendingImagesCounter
+    }
+
+    // when we create a progressBar for the second time so when .done() has been called before
+    // we create a modified tick function that automatically stops the progressbar when total is reached
+    // this is used for development as we're watching for changes
+    if (!firstPass) {
+      let progressBarCurrentValue = 0
+      const originalTickFn = progressBar.tick
+      progressBar.tick = (ticks = 1) => {
+        originalTickFn.call(progressBar, ticks)
+        progressBarCurrentValue += ticks
+
+        if (progressBarCurrentValue === pendingImagesCounter) {
+          progressBar.done()
+        }
+      }
+    }
+    firstPass = false
+  }
+
+  return progressBar
+}
+
+exports.createGatsbyProgressOrFallbackToExternalProgressBar = createGatsbyProgressOrFallbackToExternalProgressBar
+exports.createOrGetProgressBar = createOrGetProgressBar
+exports.getProgressBar = () => progressBar
