Skip to content

fix(gatsby-remark-image-contentful): show useful error message for files that can not be rendered as image #32530

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

Merged
wardpeet merged 2 commits into from
Aug 6, 2021
Merged

fix(gatsby-remark-image-contentful): show useful error message for files that can not be rendered as image #32530

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef820ca
fix: show useful error message for files that can not be rendered as …
axe312ger Jul 28, 2021
8951c3e
test(unit): improve sharp mock
axe312ger Aug 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions packages/gatsby-remark-images-contentful/src/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,28 @@ jest.mock(`../utils/`, () => {
})

jest.mock(`axios`)

let mockedSharpMetadata
jest.mock(`sharp`, () => () => {
return {
metadata: jest.fn(() => {
return {
width: 200,
height: 200,
density: 75,
}
}),
}
return { metadata: mockedSharpMetadata }
Copy link
Contributor

@wardpeet wardpeet Aug 3, 2021

Choose a reason for hiding this comment

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

Suggested change
return { metadata: mockedSharpMetadata }
return { metadata: jest.fn() }

Copy link
Contributor Author

@axe312ger axe312ger Aug 5, 2021

Choose a reason for hiding this comment

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

I changed it closer to how gatsby-plugin-sharp mocks sharp. Please have a look

})

const mockSharpSuccess = () => {
mockedSharpMetadata = jest.fn(() => {
return {
width: 200,
height: 200,
density: 75,
}
})
}

const mockSharpFailure = () => {
mockedSharpMetadata = jest.fn(() =>
Promise.reject(new Error(`invalid image`))
)
}
Copy link
Contributor

@wardpeet wardpeet Aug 3, 2021

Choose a reason for hiding this comment

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

This is the proper way to mock :)

Suggested change
const mockSharpSuccess = () => {
mockedSharpMetadata = jest.fn(() => {
return {
width: 200,
height: 200,
density: 75,
}
})
}
const mockSharpFailure = () => {
mockedSharpMetadata = jest.fn(() =>
Promise.reject(new Error(`invalid image`))
)
}
const sharp = require('sharp');
const mockSharpSuccess = () => {
sharp.metadata.mockResolvedValueOnce({
width: 200,
height: 200,
density: 75,
})
}
const mockSharpFailure = () => {
sharp.metadata.mockRejectedValue(new Error(`invalid image`)
}

Copy link
Contributor Author

@axe312ger axe312ger Aug 5, 2021

Choose a reason for hiding this comment

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

see comment above


const createNode = content => {
const node = {
id: 1234,
Expand Down Expand Up @@ -94,6 +104,7 @@ beforeEach(() => {
})

test(`it returns empty array when 0 images`, async () => {
mockSharpSuccess()
const content = `
# hello world
Look ma, no images
Expand All @@ -116,6 +127,7 @@ test(`it leaves relative images alone`, async () => {
})

test(`it leaves non-contentful images alone`, async () => {
mockSharpSuccess()
const imagePath = `//google.com/images/an-image.jpeg`
const content = `
![asdf](${imagePath})
Expand All @@ -127,6 +139,7 @@ test(`it leaves non-contentful images alone`, async () => {
})

test(`it transforms images in markdown`, async () => {
mockSharpSuccess()
const imagePath = `//images.ctfassets.net/rocybtov1ozk/wtrHxeu3zEoEce2MokCSi/73dce36715f16e27cf5ff0d2d97d7dff/quwowooybuqbl6ntboz3.jpg`
const content = `
![image](${imagePath})
Expand All @@ -143,6 +156,7 @@ test(`it transforms images in markdown`, async () => {
})

test(`it transforms images with a https scheme in markdown`, async () => {
mockSharpSuccess()
const imagePath = `https://images.ctfassets.net/rocybtov1ozk/wtrHxeu3zEoEce2MokCSi/73dce36715f16e27cf5ff0d2d97d7dff/quwowooybuqbl6ntboz3.jpg`
const content = `
![image](${imagePath})
Expand All @@ -159,6 +173,7 @@ test(`it transforms images with a https scheme in markdown`, async () => {
})

test(`it throws specific error if the image is not found`, async () => {
mockSharpSuccess()
axios.mockImplementationOnce(() => Promise.reject(new Error(`oh no`)))
const reporter = {
panic: jest.fn(),
Expand All @@ -177,6 +192,7 @@ test(`it throws specific error if the image is not found`, async () => {
})

test(`it transforms multiple images in markdown`, async () => {
mockSharpSuccess()
const imagePaths = [
`//images.ctfassets.net/rocybtov1ozk/wtrHxeu3zEoEce2MokCSi/73dce36715f16e27cf5ff0d2d97d7dff/quwowooybuqbl6ntboz3.jpg`,
`//images.ctfassets.net/rocybtov1ozk/wtrHxeu3zEoEce2MokCSi/73dce36715f16e27cf5ff0d2d97d7dff/quwowooybuqbl6ntboz3.jpg`,
Expand All @@ -193,6 +209,7 @@ test(`it transforms multiple images in markdown`, async () => {
})

test(`it transforms HTML img tags`, async () => {
mockSharpSuccess()
const imagePath = `//images.ctfassets.net/rocybtov1ozk/wtrHxeu3zEoEce2MokCSi/73dce36715f16e27cf5ff0d2d97d7dff/quwowooybuqbl6ntboz3.jpg`

const content = `
Expand All @@ -210,6 +227,7 @@ test(`it transforms HTML img tags`, async () => {
})

test(`it leaves relative HTML img tags alone`, async () => {
mockSharpSuccess()
const imagePath = `images/this-was-an-image.jpeg`

const content = `
Expand All @@ -221,6 +239,7 @@ test(`it leaves relative HTML img tags alone`, async () => {
})

test(`it transforms images in markdown with webp srcSets if option is enabled`, async () => {
mockSharpSuccess()
const imagePath = `//images.ctfassets.net/rocybtov1ozk/wtrHxeu3zEoEce2MokCSi/73dce36715f16e27cf5ff0d2d97d7dff/quwowooybuqbl6ntboz3.jpg`
const content = `
![image](${imagePath})
Expand All @@ -237,3 +256,25 @@ test(`it transforms images in markdown with webp srcSets if option is enabled`,
expect(node.value).toMatchSnapshot()
expect(node.value).not.toMatch(`<html>`)
})

test(`it shows an useful error message when the file is not a valid image`, async () => {
mockSharpFailure()

const imagePath = `//images.ctfassets.net/k8iqpp6u0ior/752jwCIe9dwtfi9mLbp9m2/bc588ee25cf8299bc33a56ca32f8677b/Gatsby-Logos.zip`

const content = `
![image](${imagePath})
`.trim()

const reporter = {
panic: jest.fn(),
}

await plugin(createPluginOptions(content, imagePath, { reporter }))

expect(reporter.panic).toHaveBeenCalledTimes(1)
expect(reporter.panic).toHaveBeenCalledWith(
`The image "${imagePath}" (with alt text: "image") doesn't appear to be a supported image format.`,
expect.any(Error)
)
})
11 changes: 10 additions & 1 deletion packages/gatsby-remark-images-contentful/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ module.exports = async (
}
const metaReader = sharp()

// @todo to increase reliablility, this should use the asset downloading function from gatsby-source-contentful
let response
try {
response = await axios({
Expand All @@ -81,7 +82,15 @@ module.exports = async (

response.data.pipe(metaReader)

const metadata = await metaReader.metadata()
let metadata
try {
metadata = await metaReader.metadata()
} catch (error) {
reporter.panic(
`The image "${node.url}" (with alt text: "${node.alt}") doesn't appear to be a supported image format.`,
error
)
}

response.data.destroy()

Expand Down