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

feat(assets): Add property to image services to control which properties to use for hashing #8984

Merged
merged 7 commits into from
Nov 8, 2023

Conversation

Princesseuh
Copy link
Member

Changes

What the title says. This is a bit advanced, and most image services won't need it so we supply a good enough default while still enabling more advanced use cases.

Fix #8956
Fix #8047

Testing

The current tests should pass! In particular, we have a test right now that tests image duplication in srcset (the most complex case). It already passed before, but it should still catch regressions

Docs

Will add it to the Image Service API page

Copy link

changeset-bot bot commented Nov 2, 2023

🦋 Changeset detected

Latest commit: 991c813

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 github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Nov 2, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I don't quite understand how this feature will fix the linked issues. Are we adding new properties to be hashed to fix them, or would they have to set it themselves? If it's the latter, are we able to do it by default instead?

I can see how this would be useful for third-party image services though since ImageTransform has [key: string]: any.

@Princesseuh
Copy link
Member Author

Princesseuh commented Nov 2, 2023

I don't quite understand how this feature will fix the linked issues. Are we adding new properties to be hashed to fix them, or would they have to set it themselves? If it's the latter, are we able to do it by default instead?

I can see how this would be useful for third-party image services though since ImageTransform has [key: string]: any.

No, we're actually removing properties! Before we'd pass everything that was passed to getImage to the hashing mechanism, since we can't know what properties a service really use (as you've discovered, since a service can really use anything. Maybe you have a fit attribute for cropping, or maybe you allow passing options to Sharp directly etc)

With this list, we now can know what properties a service actually use to impact the image, so we can add only them to the hash. Third-party services will adjust their list with the properties they need. The alternative is maintaining a blacklist, but it's very unreliable because of data attributes, proprietary attributes from third-party librairies etc.

Users don't need to do anything, this is only specific to services, and even then it's only specific to local services that don't use our properties (since the default value is the properties we use ourselves)

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Should we have at least a new test case where we use this new option?

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Left a quick suggestion to include the property name!

.changeset/new-islands-lick.md Outdated Show resolved Hide resolved
@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Nov 6, 2023
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Nov 7, 2023
@Princesseuh Princesseuh merged commit 26b1484 into main Nov 8, 2023
4 checks passed
@Princesseuh Princesseuh deleted the feat/image-service-hash branch November 8, 2023 09:10
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@astrobot-houston astrobot-houston mentioned this pull request Nov 8, 2023
@lerenos
Copy link

lerenos commented Nov 14, 2023

A small warning (probably related to ths update): I was using the remotePatterns key in image config, and it stopped optimizing my remote images after update from 3.0 to 3.5. Now I inserted the domains key and everything is working fine 🙌

@Princesseuh
Copy link
Member Author

A small warning (probably related to ths update): I was using the remotePatterns key in image config, and it stopped optimizing my remote images after update from 3.0 to 3.5. Now I inserted the domains key and everything is working fine 🙌

Shouldn't be related to this, but if you can reproduce the problem, feel free to open an issue!

natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
…ies to use for hashing (#8984)

Co-authored-by: Sarah Rainsberger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
5 participants