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

Pass reporter to gatsby-plugin-sharp #4122

Merged
merged 4 commits into from
Feb 19, 2018
Merged

Pass reporter to gatsby-plugin-sharp #4122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6615009
[gatsby-plugin-sharp] dont import reporter directly, let it be passed…
pieh Feb 18, 2018
58b4344
[gatsby-transformer-sharp] pass reporter to gatsby-plugin-sharp
pieh Feb 18, 2018
da477e8
[gatsby-remark-images] pass reporter to gatsby-plugin-sharp
pieh Feb 18, 2018
5a8f5d0
[gatsby-remark-images] bail early if gatsby-plugin-sizes couldn't gen…
pieh Feb 18, 2018
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
68 changes: 43 additions & 25 deletions packages/gatsby-plugin-sharp/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ const path = require(`path`)

const duotone = require(`./duotone`)
const { boundActionCreators } = require(`gatsby/dist/redux/actions`)
const report = require(`gatsby-cli/lib/reporter`)

// Promisify the sharp prototype (methods) to promisify the alternative (for
// raw) callback-accepting toBuffer(...) method
Expand All @@ -33,8 +32,20 @@ const bar = new ProgressBar(
}
)

const reportError = (message, err, reporter) => {
if (reporter) {
reporter.error(message, err)
} else {
console.error(message, err)
}

if (process.env.gatsby_executing_command === `build`) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is already so specific to gatsby, I don't think there is any harm in requiring the reporter to be passed to gatsby-plugin-sharp, I guess maybe it's for backwards compat though ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I wanted to backport panicOnBuild addition to reporter in gatsby-cli from v2 (which this is copied from), but thought about unofficial plugins that might be slower to update (and pass reporter) so used this as bandaid solution instead of just breaking not updated plugins.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just split out the reporting to a separate package that can be used anywhere?

I'll merge and release this PR in the meantime. But no reason to duplicate functionality. There's nothing tying the reporting to gatsby-cli.

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern wasn't the package sharing, but the singleton. Eventually to do reporting well coordinated across plugins we need more explicit control over how plugins report, since grouping output from a a bunch of different sources in parallel is tough

Copy link
Contributor

Choose a reason for hiding this comment

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

But NPM would dedupe the installation so there should be just one version of the reporter package that's running right?

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry yeah, but waht i'm saying you might not want a singleton, you may wnat to pass a "sub reporter" instance or something per plugin that only has functionality to print to it's group for instance. I think i'd need to try and demonstrate what i mean, i had made an attempt before but got sidetracked a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah gotcha.

Hmmm we could still do both right? Normal plugins would depend on the reporter passed and and the few oddballs like gatsby-plugin-sharp could depend on it directly.

process.exit(1)
}
}

let totalJobs = 0
const processFile = (file, jobs, cb) => {
const processFile = (file, jobs, cb, reporter) => {
// console.log("totalJobs", totalJobs)
bar.total = totalJobs

Expand All @@ -47,7 +58,7 @@ const processFile = (file, jobs, cb) => {
try {
pipeline = sharp(file).rotate()
} catch (err) {
report.error(`Failed to process image ${file}`, err)
reportError(`Failed to process image ${file}`, err, reporter)
jobs.forEach(job => job.outsideReject(err))
return
}
Expand Down Expand Up @@ -120,7 +131,7 @@ const processFile = (file, jobs, cb) => {
)

if (err) {
report.error(`Failed to process image ${file}`, err)
reportError(`Failed to process image ${file}`, err, reporter)
job.outsideReject(err)
} else {
job.outsideResolve()
Expand Down Expand Up @@ -181,7 +192,7 @@ const q = queue((task, callback) => {
task(callback)
}, 1)

const queueJob = job => {
const queueJob = (job, reporter) => {
const inputFileKey = job.file.absolutePath.replace(/\./g, `%2E`)
const outputFileKey = job.outputPath.replace(/\./g, `%2E`)
const jobPath = `${inputFileKey}.${outputFileKey}`
Expand Down Expand Up @@ -218,20 +229,25 @@ const queueJob = job => {
{ name: `gatsby-plugin-sharp` }
)
// We're now processing the file's jobs.
processFile(job.file.absolutePath, jobs, () => {
boundActionCreators.endJob(
{
id: `processing image ${job.file.absolutePath}`,
},
{ name: `gatsby-plugin-sharp` }
)
cb()
})
processFile(
job.file.absolutePath,
jobs,
() => {
boundActionCreators.endJob(
{
id: `processing image ${job.file.absolutePath}`,
},
{ name: `gatsby-plugin-sharp` }
)
cb()
},
reporter
)
})
}
}

function queueImageResizing({ file, args = {} }) {
function queueImageResizing({ file, args = {}, reporter }) {
const defaultArgs = {
width: 400,
quality: 50,
Expand Down Expand Up @@ -313,7 +329,7 @@ function queueImageResizing({ file, args = {} }) {
outputPath: filePath,
}

queueJob(job)
queueJob(job, reporter)

// Prefix the image src.
const prefixedSrc = options.pathPrefix + `/static` + imgSrc
Expand All @@ -329,7 +345,7 @@ function queueImageResizing({ file, args = {} }) {
}
}

async function notMemoizedbase64({ file, args = {} }) {
async function notMemoizedbase64({ file, args = {}, reporter }) {
const defaultArgs = {
width: 20,
quality: 50,
Expand All @@ -344,7 +360,7 @@ async function notMemoizedbase64({ file, args = {} }) {
try {
pipeline = sharp(file.absolutePath).rotate()
} catch (err) {
report.error(`Failed to process image ${file.absolutePath}`, err)
reportError(`Failed to process image ${file.absolutePath}`, err, reporter)
return null
}

Expand Down Expand Up @@ -398,7 +414,7 @@ async function base64(args) {
return await memoizedBase64(args)
}

async function responsiveSizes({ file, args = {} }) {
async function responsiveSizes({ file, args = {}, reporter }) {
const defaultArgs = {
maxWidth: 800,
quality: 50,
Expand All @@ -419,7 +435,7 @@ async function responsiveSizes({ file, args = {} }) {
try {
metadata = await sharp(file.absolutePath).metadata()
} catch (err) {
report.error(`Failed to process image ${file.absolutePath}`, err)
reportError(`Failed to process image ${file.absolutePath}`, err, reporter)
return null
}

Expand Down Expand Up @@ -478,6 +494,7 @@ async function responsiveSizes({ file, args = {} }) {
return queueImageResizing({
file,
args: arrrgs, // matey
reporter,
})
})

Expand All @@ -492,7 +509,7 @@ async function responsiveSizes({ file, args = {} }) {
}

// Get base64 version
const base64Image = await base64({ file, args: base64Args })
const base64Image = await base64({ file, args: base64Args, reporter })

// Construct src and srcSet strings.
const originalImg = _.maxBy(images, image => image.width).src
Expand All @@ -518,7 +535,7 @@ async function responsiveSizes({ file, args = {} }) {
}
}

async function resolutions({ file, args = {} }) {
async function resolutions({ file, args = {}, reporter }) {
const defaultArgs = {
width: 400,
quality: 50,
Expand Down Expand Up @@ -574,6 +591,7 @@ async function resolutions({ file, args = {} }) {
return queueImageResizing({
file,
args: arrrgs,
reporter,
})
})

Expand All @@ -584,7 +602,7 @@ async function resolutions({ file, args = {} }) {
}

// Get base64 version
const base64Image = await base64({ file, args: base64Args })
const base64Image = await base64({ file, args: base64Args, reporter })

const fallbackSrc = images[0].src
const srcSet = images
Expand Down Expand Up @@ -622,7 +640,7 @@ async function resolutions({ file, args = {} }) {
}
}

async function notMemoizedtraceSVG({ file, args, fileArgs }) {
async function notMemoizedtraceSVG({ file, args, fileArgs, reporter }) {
const potrace = require(`potrace`)
const trace = Promise.promisify(potrace.trace)
const defaultArgs = {
Expand All @@ -647,7 +665,7 @@ async function notMemoizedtraceSVG({ file, args, fileArgs }) {
try {
pipeline = sharp(file.absolutePath).rotate()
} catch (err) {
report.error(`Failed to process image ${file.absolutePath}`, err)
reportError(`Failed to process image ${file.absolutePath}`, err, reporter)
return null
}

Expand Down
7 changes: 6 additions & 1 deletion packages/gatsby-remark-images/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const slash = require(`slash`)
// 4. Create the responsive images.
// 5. Set the html w/ aspect ratio helper.
module.exports = (
{ files, markdownNode, markdownAST, pathPrefix, getNode },
{ files, markdownNode, markdownAST, pathPrefix, getNode, reporter },
pluginOptions
) => {
const defaults = {
Expand Down Expand Up @@ -60,8 +60,13 @@ module.exports = (
let responsiveSizesResult = await sizes({
file: imageNode,
args: options,
reporter,
})

if (!responsiveSizesResult) {
return resolve()
}

// Calculate the paddingBottom %
const ratio = `${1 / responsiveSizesResult.aspectRatio * 100}%`

Expand Down
4 changes: 3 additions & 1 deletion packages/gatsby-transformer-remark/src/extend-node-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const withPathPrefix = (url, pathPrefix) =>
(pathPrefix + url).replace(/\/\//, `/`)

module.exports = (
{ type, store, pathPrefix, getNode, cache },
{ type, store, pathPrefix, getNode, cache, reporter },
pluginOptions
) => {
if (type.name !== `MarkdownRemark`) {
Expand Down Expand Up @@ -104,6 +104,7 @@ module.exports = (
markdownNode,
files,
getNode,
reporter,
},
plugin.pluginOptions
)
Expand Down Expand Up @@ -170,6 +171,7 @@ module.exports = (
getNode,
files,
pathPrefix,
reporter,
},
plugin.pluginOptions
)
Expand Down
15 changes: 14 additions & 1 deletion packages/gatsby-transformer-sharp/src/extend-node-type.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,12 @@ const PotraceType = new GraphQLInputObjectType({
},
})

module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
module.exports = ({
type,
pathPrefix,
getNodeAndSavePathDependency,
reporter,
}) => {
if (type.name !== `ImageSharp`) {
return {}
}
Expand Down Expand Up @@ -172,6 +177,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
resolutions({
file,
args,
reporter,
})
).then(({ src }) => src)
},
Expand All @@ -187,6 +193,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
resolutions({
file,
args,
reporter,
})
).then(({ srcSet }) => srcSet)
},
Expand Down Expand Up @@ -242,6 +249,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
resolutions({
file,
args,
reporter,
})
).then(o =>
Object.assign({}, o, {
Expand Down Expand Up @@ -275,6 +283,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
sizes({
file,
args,
reporter,
})
).then(({ src }) => src)
},
Expand All @@ -290,6 +299,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
sizes({
file,
args,
reporter,
})
).then(({ srcSet }) => srcSet)
},
Expand Down Expand Up @@ -347,6 +357,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
sizes({
file,
args,
reporter,
})
).then(o =>
Object.assign({}, o, {
Expand Down Expand Up @@ -415,6 +426,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
resolutions({
file,
args,
reporter,
})
).then(o =>
Object.assign({}, o, {
Expand Down Expand Up @@ -483,6 +495,7 @@ module.exports = ({ type, pathPrefix, getNodeAndSavePathDependency }) => {
sizes({
file,
args,
reporter,
})
).then(o =>
Object.assign({}, o, {
Expand Down