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-manifest): Fix incorrect favicons size bug #12081

Merged
merged 6 commits into from
Mar 27, 2019
Merged

fix(gatsby-plugin-manifest): Fix incorrect favicons size bug #12081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1087a92
fix(gatsby-plugin-manifest): Fix incorrect favicons size bug (#12051)
keidrun Feb 25, 2019
5d01ae6
fix: non square image issue
moonmeister Mar 19, 2019
8bcd80c
fix: lint error
moonmeister Mar 22, 2019
f04ca7e
fix: cleanup warning
moonmeister Mar 22, 2019
811ecdc
refactor: move to async await
moonmeister Mar 22, 2019
534a327
test: fix tests to work with sharp.metadata()
moonmeister Mar 22, 2019
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
148 changes: 91 additions & 57 deletions packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js
View file
Open in desktop
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
Open in desktop
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 @@ -69,6 +73,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