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): strip non related args when hashing resizing args #12129

Merged
merged 9 commits into from
Mar 27, 2019

Conversation

oorestisime
Copy link
Contributor

@oorestisime oorestisime commented Feb 27, 2019

So continuing on the work to make gatsby-plugin-sharp friendly on third-party packages (oorestisime/gatsby-remark-rehype-images#2) we now face this issue #12080

TL;DR images generated from transformer-sharp or and other plugins are the same (same sha256) but with different hash (depends on the available options). The problem is that all the plugins leak their options into the hash generation and even ones that are unrelated to the resizing processing (base64, generateTraceSVG, traceSVG etc)

I've been discussing this with @pieh for the last 2-3 hours :D
Proposed solution was to sanitize the args based on a whitelist, which is implemented on the PR. Which leads to the first question:

  • is it ok if after this release all image hashes are changing?

By doing so it can also potentially help lazy image processing @wardpeet by getting rid of unrelated options when persisting jobs.

Anyway current state of the PR is that it kind of works, i ve tested this in websites using my plugin and other that not. The issue here is that there are some differences in the number of images created when this modification is on or not.

I also verified that public folder doesn't contain duplicate images:

find public -regex ".*\.\(jpg\|webp\|png\|jpeg\)" -type f -exec sha256sum {} \; | cut -d ' ' -f 1 |sort| uniq -d | wc -l

This needs to be a bit more tested (and fix update snapshots etc)

@oorestisime
Copy link
Contributor Author

oorestisime commented Feb 27, 2019

Updated metrics:

On https://github.com/baobabKoodaa/blog/tree/temp384

Without the changes 438 images. 66 duplicate images
With changes: 372 images no duplicate images. (438-66=372 TADA)

On using remark

without the changes 70 images. no duplicates
with changes 62 images

I probably hadn't cleaned public before.

@oorestisime
Copy link
Contributor Author

Minor update:

Each run is not deterministic on the number of images. Probably related to #11837
OTOH i hashed each image produced in both runs (for using-remark) and in both times we have the exact same list of sha256 and a total of 54 images.

@oorestisime
Copy link
Contributor Author

oorestisime commented Feb 27, 2019

Another update:

2 /home/grok/personal/gatsby/examples/using-remark/public/static/584f423221bf98fe367b9e4de67b8e15/75e2a/jay.jpg false
      2 /home/grok/personal/gatsby/examples/using-remark/public/static/d286acdd98800790ea1bbdddbc982962/084f1/daisy.jpg false
      2 /home/grok/personal/gatsby/examples/using-remark/public/static/d286acdd98800790ea1bbdddbc982962/25aa5/daisy.jpg false
      2 /home/grok/personal/gatsby/examples/using-remark/public/static/d286acdd98800790ea1bbdddbc982962/c814f/daisy.jpg false
      3 /home/grok/personal/gatsby/examples/using-remark/public/static/584f423221bf98fe367b9e4de67b8e15/084f1/jay.jpg false
      3 /home/grok/personal/gatsby/examples/using-remark/public/static/584f423221bf98fe367b9e4de67b8e15/25aa5/jay.jpg false
      3 /home/grok/personal/gatsby/examples/using-remark/public/static/584f423221bf98fe367b9e4de67b8e15/c814f/jay.jpg false

@oorestisime oorestisime marked this pull request as ready for review March 11, 2019 22:10
@oorestisime
Copy link
Contributor Author

I forced-push this PR rebasing to currnet master. i know jest tests will fail due to not updating snapshots. I am waiting for feedback and will then do it.

@pieh pieh changed the title Strip non related args when hashing resized images fix(gatsby-plugin-sharp): strip non related args when hashing resizing args Mar 27, 2019
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @oorestisime

@pieh pieh merged commit da0b622 into gatsbyjs:master Mar 27, 2019
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