From 366980b659a611952eaf2fc7ee73017aefd5493b Mon Sep 17 00:00:00 2001 From: Keid Date: Wed, 27 Mar 2019 04:31:42 -0700 Subject: [PATCH] fix(gatsby-plugin-manifest): Fix incorrect favicons size bug (#12081) * 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() --- .../src/__tests__/gatsby-node.js | 148 +++++++++++------- .../gatsby-plugin-manifest/src/gatsby-node.js | 21 ++- 2 files changed, 109 insertions(+), 60 deletions(-) diff --git a/packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js b/packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js index 7ae8021f9e184..718b5e5c9350e 100644 --- a/packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js +++ b/packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js @@ -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( () => @@ -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 = { @@ -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) @@ -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.` @@ -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) @@ -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) }) @@ -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) }) diff --git a/packages/gatsby-plugin-manifest/src/gatsby-node.js b/packages/gatsby-plugin-manifest/src/gatsby-node.js index f2a845a758080..fa7c3efea1375 100644 --- a/packages/gatsby-plugin-manifest/src/gatsby-node.js +++ b/packages/gatsby-plugin-manifest/src/gatsby-node.js @@ -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. @@ -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`