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): catch errors when writing base64 images #28614

Merged
merged 5 commits into from Dec 15, 2020

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Dec 14, 2020

Possibly fixes #24774 (comment)

We caught read errors for images but not write errors.

New error:
Screen Shot 2020-12-14 at 1 23 50 PM

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Dec 14, 2020
pvdz
pvdz previously approved these changes Dec 14, 2020
Copy link
Contributor

@pvdz pvdz left a comment

Choose a reason for hiding this comment

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

Great that we're now catching this one but it makes me wonder what other errors ought to be trapped that currently are not.

@KyleAMathews KyleAMathews added topic: sharp and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Dec 14, 2020
@KyleAMathews
Copy link
Contributor Author

KyleAMathews commented Dec 14, 2020

Yeah, more generic trapping makes sense to me. I'm not familiar enough with plugin-sharp's history over the past year or so to know what's been tried/considered.

@@ -1,6 +1,10 @@
const reportError = (message, err, reporter) => {
if (reporter) {
reporter.error(message, err)
reporter.error({
id: `gatsby-plugin-sharp-20000`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

without this, messages show up in the console looking like: ERROR [object Object] ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems like reporter.error broke the old-skool way of sending errors?

pvdz
pvdz previously approved these changes Dec 14, 2020
@KyleAMathews KyleAMathews added this to To cherry-pick in V2 Release hotfixes via automation Dec 14, 2020
@ascorbic
Copy link
Contributor

This could also fix #28203 and [ch19324], but may need to be moved elsewhere to make it work if the error is in another sharp operation.

@wardpeet
Copy link
Contributor

@KyleAMathews this is not really fixing the issue, this is working around it.

The root problem is here

details.context = {
sourceMessage: errorMeta + ` ` + error.message,
}

Co-authored-by: Matt Kane <matt@gatsbyjs.com>
@KyleAMathews
Copy link
Contributor Author

@wardpeet thanks for the pointer. I assumed it'd gotten broken somewhere in there but didn't have context.

@wardpeet
Copy link
Contributor

But +1 for structured logs 👍

@KyleAMathews
Copy link
Contributor Author

may need to be moved elsewhere to make it work if the error is in another sharp operation.

@ascorbic what need moved?

@ascorbic
Copy link
Contributor

@KyleAMathews The error handler. This only catches one specific sharp operation, and other operations would likely have the same error which could be uncaught if there was no base64 image

@KyleAMathews
Copy link
Contributor Author

@ascorbic can you point me to the error handler? plugin-sharp is a lot to sort through :-D Agree that's the right fix per @pvdz and my conversation above.

@ascorbic
Copy link
Contributor

I mean the one you just added!

@KyleAMathews
Copy link
Contributor Author

Oh haha I see. I thought there was a generic error handler. Yeah I'll do a quick scan through other read/write calls in plugin-sharp to see if there's any others we've missed.

@ascorbic
Copy link
Contributor

One good test would be to see what happens in your test site if you remove the base64 query. Does it bail elsewhere?

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Agreed with everyone's comments but we can iterate on that. Shipping this now since it is valuable by itself and unblocks one of my broken builds.

Thank you @KyleAMathews 🤗

Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Approving again after fixing a formatting issue

@sidharthachatterjee sidharthachatterjee merged commit 02860da into master Dec 15, 2020
@sidharthachatterjee sidharthachatterjee deleted the catch-base64-error branch December 15, 2020 13:28
@sidharthachatterjee
Copy link
Contributor

This is out on the next release channel

@KyleAMathews
Copy link
Contributor Author

Identified a few other places where we don't appear to trap sharp errors #28640

LekoArts pushed a commit that referenced this pull request Dec 16, 2020
…8614)

Co-authored-by: Matt Kane <matt@gatsbyjs.com>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Co-authored-by: Sidhartha Chatterjee <me@sidharthachatterjee.com>
(cherry picked from commit 02860da)
@LekoArts LekoArts moved this from To cherry-pick to Backport PR opened in V2 Release hotfixes Dec 16, 2020
LekoArts added a commit that referenced this pull request Dec 16, 2020
…8614) (#28659)

Co-authored-by: Matt Kane <matt@gatsbyjs.com>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Co-authored-by: Sidhartha Chatterjee <me@sidharthachatterjee.com>
(cherry picked from commit 02860da)

Co-authored-by: Kyle Mathews <mathews.kyle@gmail.com>
@LekoArts LekoArts moved this from Backport PR opened to Backported in V2 Release hotfixes Dec 16, 2020
@LekoArts
Copy link
Contributor

Published in gatsby-plugin-sharp@2.11.1

@vladar vladar moved this from Backported to Archived in V2 Release hotfixes Dec 23, 2020
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…tsbyjs#28614)

Co-authored-by: Matt Kane <matt@gatsbyjs.com>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>
Co-authored-by: Sidhartha Chatterjee <me@sidharthachatterjee.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

vips2png: "unable to write to target" error when generating thumbnails breaks the build
6 participants