-
Notifications
You must be signed in to change notification settings - Fork 10.3k
fix(gatsby-plugin-sharp): Pass failOn
to all pipelines
#37177
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
Conversation
failOn
to all pipelines
@@ -208,7 +208,7 @@ exports.pluginOptionsSchema = ({ Joi }) => | |||
), | |||
stripMetadata: Joi.boolean().default(true), | |||
defaultQuality: Joi.number().default(50), | |||
// TODO(v5): Remove deprecated failOnError option | |||
// TODO(v6): Remove deprecated failOnError option |
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.
we missed this 🙈
Hey, you closed my PR #37165 and copied most code changes into this PR, but you lost the test case, which is terrible. |
@Jeff-Tian can you please generate a smaller file? I don't want to add 4MB to this repository just for this one test case. Ideal would be below 500KB |
Yes, there are rooms to improve for #37165, but you can comment and I can change it there, instead of closing it directly. |
If it's about co-authorship for you I'm happy to add you to the commit, I just closed it because I wanted to do you a favor and fix it in a timely manner as your PR was incorrect. Going back and forth in a PR takes way more time than talking about it in advance (e.g. in a discussion) and then have a clear plan for a PR from the get go. If you give me a smaller test image I'll add it as a test case. Sorry if that made you feel unappreciated, that wasn't my intention. |
@LekoArts Thanks, feel much better now. However, I can only trigger the meta data error with the 4MB picture. If I compressed it then the meta data error won't occur. |
After talking to the author of sharp in lovell/sharp#3480 I also updated the wording on the failOn settting and the correct expectations of it. The test image you had in your PR will still fail because the image headers are corrupted. We still want to fail on this. I don't have a test file at hand that has correct headers but wrong pixel values, so we'll just assume that this is covered by sharp itself as a test case (which it is). |
Hi @LekoArts, I managed to create a very small png file that can trigger the metadata error. See #37203. You mentioned that my PR #37165 was not correctly fixed the issue, but in contrast, this PR can NOT fix the issue unless includes the #37203 in. Although the We might think that setting So I still insist passing the pluginOptions down to |
let imgStats | ||
try { | ||
const pipeline = sharp() | ||
const pipeline = sharp({ failOn: pluginOptions.failOn }) |
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.
Great, passing failOn
down to sharp
makes sense.
How about also passing the pluginOptions
down to reportErrors
so we can control the behavior of reportError
(not to exit the whole process)?
If sharp thinks an image is invalid and an error should be thrown, Allowing the option you made in the other PR is a footgun for people and would allow them to ship missing images to the production site which we don't want. |
It's not the case. See #37165's test image, it's NOT a corrupt image. It's a normal image and there are many similar images widely used. There are cases that a gatsby site contains many images, and only a few normal images happen to trigger the And the If you want the whole process to exit when |
Please open an issue on https://github.com/lovell/sharp/issues about that specific image then, sharp thinks it's corrupt so we'll handle it that way. |
Could you please add me to the commit? @LekoArts |
Co-authored By: Jeff Tian <jeff.tian@outlook.com>
@Jeff-Tian Done 👍 |
Hi @LekoArts, sorry for the late response to this issue. With the help of the author of And that makes I found ways to detect these images (lovell/sharp#806 (comment)). I am still trying to make my gatsby site building process continue even with those Is it a correct way to insert these kind of logic to Thanks in advance! |
Any updates on handling bmp images during gatsby building? Thank you |
Description
#37165 tried to fix an error that really should be fixed by correctly passing
failOn
to all our sharp pipelines.Related Issues
#37151
[ch59059]