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-source-contentful): Improve base64 placeholders #29034

Merged
merged 4 commits into from
Jan 15, 2021
Merged

fix(gatsby-source-contentful): Improve base64 placeholders #29034

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ab44dc
fix: prevent huge images from being embedded as base64
axe312ger Jan 14, 2021
d529588
fix: replace unmaintained base64-img with native solutions
axe312ger Jan 14, 2021
e9b9da4
fixup! fix: replace unmaintained base64-img with native solutions
axe312ger Jan 15, 2021
88b9861
fixup! fix: prevent huge images from being embedded as base64
axe312ger Jan 15, 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
2 changes: 0 additions & 2 deletions packages/gatsby-source-contentful/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
"@contentful/rich-text-types": "^14.1.2",
"@hapi/joi": "^15.1.1",
"axios": "^0.21.1",
"base64-img": "^1.0.4",
"bluebird": "^3.7.2",
"chalk": "^4.1.0",
"contentful": "^8.1.7",
"fs-extra": "^9.0.1",
Expand Down
50 changes: 27 additions & 23 deletions packages/gatsby-source-contentful/src/extend-node-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const fs = require(`fs`)
const path = require(`path`)
const crypto = require(`crypto`)

const Promise = require(`bluebird`)
const axios = require(`axios`)
const {
GraphQLObjectType,
GraphQLBoolean,
Expand All @@ -12,7 +12,6 @@ const {
GraphQLNonNull,
} = require(`gatsby/graphql`)
const qs = require(`qs`)
const base64Img = require(`base64-img`)

const cacheImage = require(`./cache-image`)

Expand Down Expand Up @@ -63,6 +62,11 @@ const getBase64Image = imageProps => {
return null
}

// We only support images that are delivered thourgh Contentful's Image API
if (imageProps.baseUrl.indexOf(`images.ctfassets.net`) === -1) {
return null
}

const requestUrl = `https:${imageProps.baseUrl}?w=20`

// Prefer to return data sync if we already have it
Expand Down Expand Up @@ -98,19 +102,25 @@ const getBase64Image = imageProps => {
})
}

const promise = new Promise((resolve, reject) => {
base64Img.requestBase64(requestUrl, (a, b, body) => {
// TODO: against dogma, confirm whether writeFileSync is indeed slower
fs.promises
.writeFile(cacheFile, body)
.then(() => resolve(body))
.catch(e => {
console.error(
`Contentful:getBase64Image: failed to write ${body.length} bytes remotely fetched from \`${requestUrl}\` to: \`${cacheFile}\`\nError: ${e}`
)
reject(e)
})
const promise = new Promise(async (resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be a need to wrap this all in a Promise. It's an async function so it returns a promise anyway, and you can just await the fs-extra calls.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need the promise for the caching layer. Otherwise I'd have removed it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See: https://github.com/gatsbyjs/gatsby/pull/29034/files#diff-55db4898b8c61cf48dade29472a46c02900ba554718a7fe91cc2460a61235e26R126

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but async fucntions return promises anyway, so no need to wrap it like you've done.

Copy link
Contributor

Choose a reason for hiding this comment

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

async function loadImage() {

..then:

const promise = loadImage()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well yeah I can make a small function that is called. I tried not to alter the code structure to much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ascorbic done

const imageResponse = await axios.get(requestUrl, {
responseType: `arraybuffer`,
})

const base64 = Buffer.from(imageResponse.data, `binary`).toString(`base64`)

const body = `data:image/jpeg;base64,${base64}`

// TODO: against dogma, confirm whether writeFileSync is indeed slower
fs.promises
Copy link
Contributor

Choose a reason for hiding this comment

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

You have fs-extra in the project which promisifies all of the fs calls, so you can just use that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But why go fs-extra if we have it supported in the language?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is marked as stable api:

https://nodejs.org/dist/latest-v10.x/docs/api/fs.html#fs_fs_promises_api

.writeFile(cacheFile, body)
.then(() => resolve(body))
.catch(e => {
console.error(
`Contentful:getBase64Image: failed to write ${body.length} bytes remotely fetched from \`${requestUrl}\` to: \`${cacheFile}\`\nError: ${e}`
)
reject(e)
})
})

inFlightBase64Cache.set(requestUrl, promise)
Expand Down Expand Up @@ -398,9 +408,7 @@ const fixedNodeType = ({ name, getTracedSVG }) => {
fields: {
base64: {
type: GraphQLString,
resolve(imageProps) {
return getBase64Image(imageProps)
},
resolve: getBase64Image,
},
tracedSVG: {
type: GraphQLString,
Expand Down Expand Up @@ -495,9 +503,7 @@ const fluidNodeType = ({ name, getTracedSVG }) => {
fields: {
base64: {
type: GraphQLString,
resolve(imageProps) {
return getBase64Image(imageProps)
},
resolve: getBase64Image,
},
tracedSVG: {
type: GraphQLString,
Expand Down Expand Up @@ -642,9 +648,7 @@ exports.extendNodeType = ({ type, store, cache, getNodesByType }) => {
fields: {
base64: {
type: GraphQLString,
resolve(imageProps) {
return getBase64Image(imageProps)
},
resolve: getBase64Image,
},
tracedSVG: {
type: GraphQLString,
Expand Down