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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby-plugin-sharp): catch errors when writing base64 images #28614

Merged
merged 5 commits into from
Dec 15, 2020
Merged

fix(gatsby-plugin-sharp): catch errors when writing base64 images #28614

merged 5 commits into from
Dec 15, 2020

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Dec 14, 2020 •
edited
Loading

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
@KyleAMathews KyleAMathews mentioned this pull request Dec 14, 2020
Closed
pvdz
pvdz previously approved these changes Dec 14, 2020
View reviewed changes
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 •
edited
Loading

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.

KyleAMathews
KyleAMathews commented Dec 14, 2020
View reviewed changes
packages/gatsby-plugin-sharp/src/report-error.js
@@ -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
View reviewed changes
@KyleAMathews KyleAMathews mentioned this pull request Dec 15, 2020
Closed
@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

gatsby/packages/gatsby-cli/src/reporter/reporter.ts

Lines 128 to 130 in 4cfc0bb

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

@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?

sidharthachatterjee
sidharthachatterjee previously approved these changes Dec 15, 2020
View reviewed changes
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 🤗

sidharthachatterjee
sidharthachatterjee approved these changes Dec 15, 2020
View reviewed changes
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 KyleAMathews mentioned this pull request Dec 15, 2020
Closed
@KyleAMathews
Copy link
Contributor Author

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

@LekoArts LekoArts mentioned this pull request Dec 16, 2020
Merged
LekoArts pushed a commit that referenced this pull request Dec 16, 2020
@KyleAMathews @LekoArts
fix(gatsby-plugin-sharp): catch errors when writing base64 images (#2…
79d78cd 
…8614)

Co-authored-by: Matt Kane <[email protected]>
Co-authored-by: gatsbybot <[email protected]>
Co-authored-by: Sidhartha Chatterjee <[email protected]>
(cherry picked from commit 02860da)
LekoArts added a commit that referenced this pull request Dec 16, 2020
@LekoArts @KyleAMathews
fix(gatsby-plugin-sharp): catch errors when writing base64 images (#2…
ea741c3 
…8614) (#28659)

Co-authored-by: Matt Kane <[email protected]>
Co-authored-by: gatsbybot <[email protected]>
Co-authored-by: Sidhartha Chatterjee <[email protected]>
(cherry picked from commit 02860da)

Co-authored-by: Kyle Mathews <[email protected]>
@LekoArts
Copy link
Contributor

Published in [email protected]

@billfienberg billfienberg mentioned this pull request Dec 16, 2020
Closed
2 tasks
This was referenced Dec 17, 2020
Closed
Closed
@barbalex barbalex mentioned this pull request Jan 20, 2021
Closed
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
@gatsbybot @ascorbic @sidharthachatterjee
fix(gatsby-plugin-sharp): catch errors when writing base64 images (ga…
91e07eb 
…tsbyjs#28614)

Co-authored-by: Matt Kane <[email protected]>
Co-authored-by: gatsbybot <[email protected]>
Co-authored-by: Sidhartha Chatterjee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ascorbic ascorbic ascorbic left review comments

@sidharthachatterjee sidharthachatterjee sidharthachatterjee approved these changes

@pvdz pvdz pvdz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
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
@KyleAMathews @ascorbic @wardpeet @sidharthachatterjee @LekoArts @pvdz