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

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

Merged
merged 4 commits into from
Jan 27, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jan 24, 2020

Note: This is very hacky - primarly the jobs v2 handling:
plugins right now don't know if they create duplicate jobs - this is internal gatsby core feature that is not exposed to plugins, so plugins potentially would need to implement duplicate jobs tracking themselves (duplicate functionality already built into gatsby). This is not ideal because implementation might not end up 1:1 (or implementations can drift over time)

Instead I listen to internal actions here, which are guaranteed not be emitted for duplicates (for jobs v2 code path). Legacy jobs / jobsv1 don't have any of this and this requires plugins to track things, so we already do that in sharp plugin - I just moved progress bar handling to "better" location so it doesn't display duplicates.

Long/medium term we should have Jobs v2 able to manage progress bar, but currently we are lacking some APIs for this:

  • what should be label of progress bar?
  • how many "ticks" each job should add to totals / increment progress bar when job finishes
  • should a job even show progress bar (i can see some internals using jobs later on that might not be very useful to show to users)?

fixes #20816

---edited:
we will need new API to provide some metadata for jobs created from same plugin - see #20839 (comment) as general direction

Because of that we don't need to preemptively make any checks, because adding support for progress bar management by jobs API in core won't immediately result in progress bar showing by plugin creating v2 jobs. The check will be added once API is designed and implemented in core, so there shouldn't be any weird regressions like 2 progress bars for same thing or none progress bars

…rogress bar reporting duplicates that don't do actual processing
pendingImagesCounter = 0
}

progressBar.addImageToProcess = imageCount => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what we had in index file previously (moved it to utils because it's imported in few locations, so this avoids circular imports)

There is just addition of convenience addImageToProcess method to increase total (and start activity if needed) which would need to be copied to two places if it was not put here (or some other helper function)

@KyleAMathews
Copy link
Contributor

👍 to hot fix and having jobs itself drive the progress bar. I've imagined an API where the job creator can call e.g. setJobTypeMetadata where they can add labels etc.

@pieh
Copy link
Contributor Author

pieh commented Jan 24, 2020

I've imagined an API where the job creator can call e.g. setJobTypeMetadata where they can add labels etc.

Just to clarify - this would be an action (possibly available only in onPreBootstrap or one of those guaranteed to run only once early enough)? That would make it easy to determine if we need hacky code path (with emitter listening) or not:

exports.onPreBootstrap = ({ actions, emitter }) => {
  if (actions.setJobTypeMetadata) {
    // use the action to set metadata
    // example (concrete API design is not important - just the name of it for now)
    actions.setJobTypeMetadata({
      name: `IMAGE_PROCESSING`,
      label: `Generating image thumbnails`,
      ticksFromJob: job => job.args.operations.length
    })
  } else if (emitter) {
   // use hacks/internal actions to manage progress bar
  }
}

@pieh pieh marked this pull request as ready for review January 26, 2020 22:14
@pieh pieh requested a review from a team as a code owner January 26, 2020 22:14
@pieh pieh requested a review from wardpeet January 26, 2020 22:18
@KyleAMathews
Copy link
Contributor

Yeah exactly.

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

It works on latest gatsby and older versions of gatsby so 👍

Thanks @pieh for fixing this one, this will lower the confusion for many users

@wardpeet wardpeet merged commit db36d69 into master Jan 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the sharp-progress-bar-confussion branch January 27, 2020 11:25
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.

[gatsby-plugin-sharp] Regression of the thumbnail generation bug
3 participants