Skip to content

Commit

Permalink
fix(gatsby-plugin-manifest): Fix incorrect favicons size bug (#12081)
Browse files Browse the repository at this point in the history 
* fix(gatsby-plugin-manifest): Fix incorrect favicons size bug (#12051)

Fix #12051 issue

* fix: non square image issue

    add height and width paramaters to sharp
    change fit to 'cover' to not malform image
    set background to be transparent to eliminate black bars
    add better logging to warn user when src image isn't square.

* fix: lint error

* fix: cleanup warning

* refactor: move to async await

* test: fix tests to work with sharp.metadata()
  • Loading branch information
@keidrun @wardpeet
keidrun authored and wardpeet committed Mar 27, 2019
1 parent 127f232 commit 366980b
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 60 deletions.
148 changes: 91 additions & 57 deletions packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ jest.mock(`fs`, () => {
* We mock sharp because it depends on fs implementation (which is mocked)
* this causes test failures, so mock it to avoid
*/

jest.mock(`sharp`, () => {
let sharp = jest.fn(
() =>
Expand All @@ -20,16 +21,31 @@ jest.mock(`sharp`, () => {
toFile() {
return Promise.resolve()
}
metadata() {
return {
width: 128,
height: 128,
}
}
}()
)
sharp.simd = jest.fn()
sharp.concurrency = jest.fn()

return sharp
})

const fs = require(`fs`)
const path = require(`path`)
const sharp = require(`sharp`)
const reporter = {
activityTimer: jest.fn().mockImplementation(function() {
return {
start: jest.fn(),
end: jest.fn(),
}
}),
}
const { onPostBootstrap } = require(`../gatsby-node`)

const manifestOptions = {
Expand Down Expand Up @@ -60,14 +76,17 @@ describe(`Test plugin manifest options`, () => {
})

it(`correctly works with default parameters`, async () => {
await onPostBootstrap([], {
name: `GatsbyJS`,
short_name: `GatsbyJS`,
start_url: `/`,
background_color: `#f7f0eb`,
theme_color: `#a2466c`,
display: `standalone`,
})
await onPostBootstrap(
{ reporter },
{
name: `GatsbyJS`,
short_name: `GatsbyJS`,
start_url: `/`,
background_color: `#f7f0eb`,
theme_color: `#a2466c`,
display: `standalone`,
}
)
const [filePath, contents] = fs.writeFileSync.mock.calls[0]
expect(filePath).toEqual(path.join(`public`, `manifest.webmanifest`))
expect(sharp).toHaveBeenCalledTimes(0)
Expand All @@ -80,46 +99,52 @@ describe(`Test plugin manifest options`, () => {
const icon = `pretend/this/exists.png`
const size = 48

await onPostBootstrap([], {
name: `GatsbyJS`,
short_name: `GatsbyJS`,
start_url: `/`,
background_color: `#f7f0eb`,
theme_color: `#a2466c`,
display: `standalone`,
icon,
icons: [
{
src: `icons/icon-48x48.png`,
sizes: `${size}x${size}`,
type: `image/png`,
},
],
})
await onPostBootstrap(
{ reporter },
{
name: `GatsbyJS`,
short_name: `GatsbyJS`,
start_url: `/`,
background_color: `#f7f0eb`,
theme_color: `#a2466c`,
display: `standalone`,
icon,
icons: [
{
src: `icons/icon-48x48.png`,
sizes: `${size}x${size}`,
type: `image/png`,
},
],
}
)

expect(sharp).toHaveBeenCalledWith(icon, { density: size })
expect(sharp).toHaveBeenCalledTimes(1)
expect(sharp).toHaveBeenCalledTimes(2)
})

it(`fails on non existing icon`, async () => {
fs.statSync.mockReturnValueOnce({ isFile: () => false })

return onPostBootstrap([], {
name: `GatsbyJS`,
short_name: `GatsbyJS`,
start_url: `/`,
background_color: `#f7f0eb`,
theme_color: `#a2466c`,
display: `standalone`,
icon: `non/existing/path`,
icons: [
{
src: `icons/icon-48x48.png`,
sizes: `48x48`,
type: `image/png`,
},
],
}).catch(err => {
return onPostBootstrap(
{ reporter },
{
name: `GatsbyJS`,
short_name: `GatsbyJS`,
start_url: `/`,
background_color: `#f7f0eb`,
theme_color: `#a2466c`,
display: `standalone`,
icon: `non/existing/path`,
icons: [
{
src: `icons/icon-48x48.png`,
sizes: `48x48`,
type: `image/png`,
},
],
}
).catch(err => {
expect(sharp).toHaveBeenCalledTimes(0)
expect(err).toBe(
`icon (non/existing/path) does not exist as defined in gatsby-config.js. Make sure the file exists relative to the root of the site.`
Expand All @@ -135,10 +160,13 @@ describe(`Test plugin manifest options`, () => {
theme_color_in_head: false,
cache_busting_mode: `name`,
}
await onPostBootstrap([], {
...manifestOptions,
...pluginSpecificOptions,
})
await onPostBootstrap(
{ reporter },
{
...manifestOptions,
...pluginSpecificOptions,
}
)
expect(sharp).toHaveBeenCalledTimes(0)
const content = JSON.parse(fs.writeFileSync.mock.calls[0][1])
expect(content).toEqual(manifestOptions)
Expand All @@ -152,12 +180,15 @@ describe(`Test plugin manifest options`, () => {
legacy: true,
cache_busting_mode: `name`,
}
await onPostBootstrap([], {
...manifestOptions,
...pluginSpecificOptions,
})

expect(sharp).toHaveBeenCalledTimes(1)
await onPostBootstrap(
{ reporter },
{
...manifestOptions,
...pluginSpecificOptions,
}
)

expect(sharp).toHaveBeenCalledTimes(2)
const content = JSON.parse(fs.writeFileSync.mock.calls[0][1])
expect(content).toEqual(manifestOptions)
})
Expand All @@ -170,12 +201,15 @@ describe(`Test plugin manifest options`, () => {
legacy: true,
cache_busting_mode: `none`,
}
await onPostBootstrap([], {
...manifestOptions,
...pluginSpecificOptions,
})

expect(sharp).toHaveBeenCalledTimes(1)
await onPostBootstrap(
{ reporter },
{
...manifestOptions,
...pluginSpecificOptions,
}
)

expect(sharp).toHaveBeenCalledTimes(2)
const content = JSON.parse(fs.writeFileSync.mock.calls[0][1])
expect(content).toEqual(manifestOptions)
})
Expand Down
21 changes: 18 additions & 3 deletions packages/gatsby-plugin-manifest/src/gatsby-node.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,17 @@ function generateIcons(icons, srcIcon) {
// Sharp accept density from 1 to 2400
const density = Math.min(2400, Math.max(1, size))
return sharp(srcIcon, { density })
.resize(size)
.resize({
width: size,
height: size,
fit: `contain`,
background: { r: 255, g: 255, b: 255, alpha: 0 },
})
.toFile(imgPath)
.then(() => {})
})
}

exports.onPostBootstrap = async (args, pluginOptions) => {
exports.onPostBootstrap = async ({ reporter }, pluginOptions) => {
const { icon, ...manifest } = pluginOptions

// Delete options we won't pass to the manifest.webmanifest.
Expand Down Expand Up @@ -74,6 +78,17 @@ exports.onPostBootstrap = async (args, pluginOptions) => {
throw `icon (${icon}) does not exist as defined in gatsby-config.js. Make sure the file exists relative to the root of the site.`
}

let sharpIcon = sharp(icon)

let metadata = await sharpIcon.metadata()

if (metadata.width !== metadata.height) {
reporter.warn(
`The icon(${icon}) you provided to 'gatsby-plugin-manifest' is not square.\n` +
`The icons we generate will be square and for the best results we recommend you provide a square icon.\n`
)
}

//add cache busting
const cacheMode =
typeof pluginOptions.cache_busting_mode !== `undefined`
Expand Down

0 comments on commit 366980b

Please sign in to comment.