fix: remote srcset images not being resized and deduplication not working in certain cases #8823
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: b0444b9 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
pkg: astro
Princesseuh
commented
Oct 13, 2023
Comment on lines
+268
to
+269
| // Caution: The logic below is a bit tricky, as we need to make sure we don't generate the same image multiple times | ||
| // When making changes, make sure to test with different combinations of local/remote images widths, densities, and dimensions etc. |
There was a problem hiding this comment.
Added a "There be dragons" comment here because it's honestly super tricky to completely avoid making transforms that are functionally equivalent to another one, but don't have quite the exact same shape.
Common examples includes:
- A transform where
webpis specified manually vs one where it's not. Due to the default, both of those results in the same image. - A transform where
widthis specified, but it's equivalent to the width of the image, so it's not resized.
Princesseuh
commented
Oct 13, 2023
| @@ -1,3 +1,4 @@ | |||
| import { deterministicString } from 'deterministic-object-hash'; | |||
There was a problem hiding this comment.
I found this package in the backwoods somewhere in a random stack overflow thread, but it's honestly quite nice and does everything I wanted.
|
!preview srcset-remote |
|
Princesseuh
changed the title
|
Is there any chance to know when is this gonna get merged? It's a major bug fix on the newly announced feature... I know this issue exists so I won't use |
We'll release it Monday morning CET, it needs to get reviewed and we don't typically do releases on the weekend |
ematipico
approved these changes
Oct 16, 2023
Sorry, I tried making it more readable in this PR than the initial code, but it's kinda hard to make neat. Images are complicated 😔 I added a bunch of comments to clarify some parts, in addition to clarifying the points you commented on, hope it helps. |
|
It's okay, you did a great job! Thank you for addressing my comments. Feel free to merge it whenever you want. I don't have big concerns |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Continuation of #8819
This PR also reworks completely how we do the transforms for
srcset, putting special care into avoiding regenerating images when unnecessary. It's possible that there's still some edge cases where it doesn't work, nonetheless as far as I could tell, it does work!This also fixes #8839, since it's in the same area.
Testing
Added tests!
Docs
N/A