Skip to content

Commit

Permalink
feat(gatsby-sharp): create more resilient wrapper around sharp (#34339)
Browse files Browse the repository at this point in the history
* feat(gatsby-sharp): add gatsby-sharp as sharp abstraction

* move safe-sharp to gatsby/sharp when available

* fix yarn.lock and snapshots because of deps

* only require sharp once + typescript fixes

* fix fallback safe-sharp

* update sharp

* fix jest babel resolution

* Apply suggestions from code review
  • Loading branch information
wardpeet committed Jan 18, 2022
1 parent d319983 commit a3fa646
Show file tree
Hide file tree
Showing 17 changed files with 258 additions and 71 deletions.
7 changes: 7 additions & 0 deletions jest-transformer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,11 @@ const babelJest = require(`babel-jest`)

module.exports = babelJest.default.createTransformer({
presets: [`babel-preset-gatsby-package`],
babelrcRoots: [
// Keep the root as a root
`.`,

// Also consider monorepo packages "root" and load their .babelrc files.
`./packages/*`,
],
})
5 changes: 0 additions & 5 deletions packages/gatsby-plugin-manifest/src/__tests__/gatsby-node.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,6 @@ describe(`Test plugin manifest options`, () => {
sharp.mockClear()
})

// the require of gatsby-node performs the invoking
it(`invokes sharp.simd for optimization`, () => {
expect(sharp.simd).toHaveBeenCalledTimes(1)
})

it(`correctly works with default parameters`, async () => {
await onPostBootstrap(apiArgs, {
name: `GatsbyJS`,
Expand Down
10 changes: 4 additions & 6 deletions packages/gatsby-plugin-manifest/src/gatsby-node.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
import * as fs from "fs"
import * as path from "path"
import sharp from "./safe-sharp"
// TODO(v5): use gatsby/sharp
import getSharpInstance from "./safe-sharp"
import { createContentDigest, slash } from "gatsby-core-utils"
import { defaultIcons, addDigestToPath, favicons } from "./common"
import { doesIconExist } from "./node-helpers"

import pluginOptionsSchema from "./pluginOptionsSchema"

sharp.simd(true)

// force it to be 1 as we only resize one image
sharp.concurrency(1)

async function generateIcon(icon, srcIcon) {
const imgPath = path.join(`public`, icon.src)

Expand All @@ -28,6 +24,7 @@ async function generateIcon(icon, srcIcon) {
// Sharp accept density from 1 to 2400
const density = Math.min(2400, Math.max(1, size))

const sharp = await getSharpInstance()
return sharp(srcIcon, { density })
.resize({
width: size,
Expand Down Expand Up @@ -194,6 +191,7 @@ const makeManifest = async ({
)
}

const sharp = await getSharpInstance()
const sharpIcon = sharp(icon)

const metadata = await sharpIcon.metadata()
Expand Down
12 changes: 11 additions & 1 deletion packages/gatsby-plugin-manifest/src/safe-sharp.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,17 @@ try {
originalConsoleError(msg, ...args)
handleMessage(msg)
}
sharp = require(`sharp`)
try {
sharp = require(`gatsby/sharp`)
} catch (e) {
sharp = () => {
const sharp = require(`sharp`)
sharp.simd()
sharp.concurrency(1)

return Promise.resolve(sharp)
}
}
} catch (e) {
handleMessage(e.toString())
throw e
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,12 @@ jest.mock(`sharp`, () => {
const pipeline = {
metadata: metadataMock,
}

return pipeline
}

sharp.simd = jest.fn()
sharp.concurrency = jest.fn()
sharp.metadataMock = metadataMock

return sharp
Expand Down
5 changes: 4 additions & 1 deletion packages/gatsby-remark-images-contentful/src/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const { selectAll } = require(`unist-util-select`)
const sharp = require(`./safe-sharp`)
// TODO(v5): use gatsby/sharp
const getSharpInstance = require(`./safe-sharp`)
const axios = require(`axios`)
const _ = require(`lodash`)
const Promise = require(`bluebird`)
Expand Down Expand Up @@ -62,6 +63,7 @@ module.exports = async (
if (cachedRawHTML) {
return cachedRawHTML
}
const sharp = await getSharpInstance()
const metaReader = sharp()

// @todo to increase reliablility, this should use the asset downloading function from gatsby-source-contentful
Expand All @@ -86,6 +88,7 @@ module.exports = async (
try {
metadata = await metaReader.metadata()
} catch (error) {
console.log(error)
reporter.panic(
`The image "${node.url}" (with alt text: "${node.alt}") doesn't appear to be a supported image format.`,
error
Expand Down
13 changes: 12 additions & 1 deletion packages/gatsby-remark-images-contentful/src/safe-sharp.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,18 @@ try {
originalConsoleError(msg, ...args)
handleMessage(msg)
}
sharp = require(`sharp`)
try {
sharp = require(`gatsby/sharp`)
} catch (e) {
sharp = () => {
const sharp = require(`sharp`)

sharp.simd()
sharp.concurrency(1)

return Promise.resolve(sharp)
}
}
} catch (e) {
handleMessage(e.toString())
throw e
Expand Down
5 changes: 5 additions & 0 deletions packages/gatsby-sharp/.babelrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
console.log('hi there');
module.exports = {
"presets": [["babel-preset-gatsby-package"]],
"plugins": ["babel-plugin-replace-ts-export-assignment"]
}
41 changes: 41 additions & 0 deletions packages/gatsby-sharp/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"name": "gatsby-sharp",
"version": "0.0.1-next.0",
"sideEffects": false,
"keywords": [
"gatsby",
"sharp"
],
"main": "dist/index.js",
"source": "src/index.ts",
"files": [
"dist/*"
],
"types": "dist/index.d.ts",
"homepage": "https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby-sharp#readme",
"dependencies": {
"@types/sharp": "^0.29.5",
"sharp": "^0.29.3"
},
"devDependencies": {
"@babel/cli": "^7.15.5",
"@babel/core": "^7.15.5",
"babel-plugin-replace-ts-export-assignment": "^0.0.2",
"cross-env": "^7.0.3"
},
"engines": {
"node": ">=14.15.0"
},
"repository": {
"type": "git",
"url": "https://github.com/gatsbyjs/gatsby.git",
"directory": "packages/gatsby-sharp"
},
"license": "MIT",
"scripts": {
"build": "babel src --out-file dist/index.js --ignore \"**/__tests__\" --extensions \".ts,.js\"",
"typegen": "tsc --emitDeclarationOnly --declaration --declarationDir dist/",
"prepare": "cross-env NODE_ENV=production npm-run-all -s build typegen",
"watch": "babel -w src --out-file dist/index.js --ignore \"**/__tests__\" --extensions \".ts,.js\""
}
}
76 changes: 76 additions & 0 deletions packages/gatsby-sharp/src/__tests__/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/** @jest-environment node */
import { exec } from "child_process"

jest.mock(`child_process`, () => {
return {
exec: jest.fn(async (command, options, cb) => {
setImmediate(() => {
try {
return cb(
null,
`> sharp@0.29.3 install C:\\Users\\Ward\\projects\\gatsby\\gatsby\\node_modules\\sharp\n`,
``
)
} catch (err) {
return cb(true, ``, err.message)
}
})
}),
}
})

function getSharpInstance(): typeof import("../index") {
let getSharpInstance
jest.isolateModules(() => {
getSharpInstance = require(`../index`)
})

return getSharpInstance()
}

describe(`getSharpInstance`, () => {
beforeEach(() => {
exec.mockClear()
})

// jest mocking is making this impossible to test
it(`should give you the bare sharp module`, async () => {
const sharpInstance = await getSharpInstance()

expect(exec).not.toHaveBeenCalled()
expect(sharpInstance).toBeDefined()
expect(sharpInstance.versions).toBeDefined()
})

it(
`should rebuild sharp when binaries not found for current arch`,
async () => {
expect.assertions(3)

let called = false
jest.doMock(`sharp`, () => {
if (!called) {
called = true
throw new Error(`sharp failed to load`)
}

return jest.requireActual(`sharp`)
})

try {
const sharpInstance = await getSharpInstance()
expect(sharpInstance).toBeDefined()
expect(sharpInstance.versions).toBeDefined()
} catch (err) {
// ignore
}

expect(exec).toHaveBeenCalledWith(
`npm rebuild sharp`,
expect.anything(),
expect.anything()
)
},
60 * 1000
)
})
52 changes: 52 additions & 0 deletions packages/gatsby-sharp/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
const { exec } = require(`child_process`)
const { createRequire } = require(`module`)

let sharpInstance: typeof import("sharp")

export = async function getSharpInstance(): Promise<typeof import("sharp")> {
try {
return importSharp()
} catch (err) {
await rebuildSharp()

// Try importing again now we have rebuilt sharp
return importSharp()
}
}

function importSharp(): typeof import("sharp") {
if (!sharpInstance) {
const cleanRequire = createRequire(__filename)
const sharp = cleanRequire(`sharp`)

sharp.simd(true)
// Concurrency is handled by gatsby
sharp.concurrency(1)

sharpInstance = sharp
}

return sharpInstance
}

function rebuildSharp(): Promise<string> {
return new Promise((resolve, reject) => {
exec(
`npm rebuild sharp`,
{
timeout: 60 * 1000,
},
(error, stdout, stderr) => {
if (error) {
if (error.killed) {
console.log(`timeout reached`)
}

return reject(stderr)
}

return setImmediate(() => resolve(stdout))
}
)
})
}
12 changes: 12 additions & 0 deletions packages/gatsby-sharp/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"target": "ES5",
"module": "CommonJS"
},
"exclude": [
"src/__tests__",
"src/__mocks__",
"dist",
]
}
2 changes: 1 addition & 1 deletion packages/gatsby-source-contentful/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"gatsby": "^4.0.0-next",
"gatsby-plugin-image": "^2.0.0-next",
"gatsby-plugin-sharp": "^4.0.0-next",
"sharp": "^0.29.0"
"sharp": "^0.29.3"
},
"repository": {
"type": "git",
Expand Down
4 changes: 4 additions & 0 deletions packages/gatsby/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@
"@types/reach__router": "^1.3.5",
"@types/react-dom": "^17.0.9",
"@types/semver": "^7.3.9",
"@types/sharp": "^0.29.5",
"@types/signal-exit": "^3.0.0",
"@types/string-similarity": "^4.0.0",
"@types/tmp": "^0.2.0",
Expand All @@ -188,6 +189,9 @@
"zipkin-javascript-opentracing": "^3.0.0",
"zipkin-transport-http": "^0.22.0"
},
"optionalDependencies": {
"gatsby-sharp": "^0.0.1-next.0"
},
"engines": {
"node": ">=14.15.0"
},
Expand Down
3 changes: 3 additions & 0 deletions packages/gatsby/sharp.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import getSharpInstance from "gatsby-sharp"

export = getSharpInstance
3 changes: 3 additions & 0 deletions packages/gatsby/sharp.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
"use strict"

module.exports = require('gatsby-sharp');
Loading

0 comments on commit a3fa646

Please sign in to comment.