-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: allow sharp format options #13073
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
CodSpeed Performance ReportMerging #13073 will not alter performanceComparing Summary
|
@Ceres6 if there has been a discussion on Discord, it would be useful to link here the thread/topic/message, so it will provides us more context. |
I don't see anything wrong in adding more options, however since the PR lacks context, and it doesn't describe how these options will be used by our end users (lack of tests, changeset or even description), I find it very difficult to give a proper review. |
Here is the thread although it wasn't a long discussion. The context is that for some webs where image quality is critical having those extra options would be great. My use case is a photographers portfolio where compression to avif in images with smooth white backgrounds appear riddles, so options as The change is to be able to pass that in config like: import { defineConfig } from 'astro/config'
export default defineConfig({
image: {
// Example: Enable the Sharp-based image service with a custom config
service: {
entrypoint: 'astro/assets/services/sharp',
config: {
formatConfig: {
avif: {
effort: 9,
lossless: true,
},
},
},
},
},
}) |
@ematipico do you need something else to review? |
Yes. Since this is a minor, we need to add:
You should add some tests that cover the usage of these new options. As for the documentation, you will need to create a PR to document these new options. |
🦋 Changeset detectedLatest commit: 0544280 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 |
Any idea on how to test it? I mean, I can create a test using them but the result will be a new image generated so not sure if not having an error is enough? I could build the same image with sharp directly and check they're equal, I guess |
Changes
Allow sharp format options (e.g. https://sharp.pixelplumbing.com/api-output#avif)
Testing
I haven't added any tests yet, just opening to see if the feature is desired and this a good approach. However I'm not sure how to test this, trying all combinations would be impossible, maybe mocking to check options are actually passed is an option
Docs
I will add the change to the config reference once the api is decided.
cc @Princesseuh as discussed on Discord