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.

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

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Feb 18, 2018

Fix reporter dependency issue as suggested in #4118 (comment)

pieh added 4 commits February 18, 2018 20:16
… is function paramater

add helper function that will use reporter if supplied or standard console.error if not
add panic on build as we don't want build successfuly if there were errors during image processing
@ghost ghost assigned pieh Feb 18, 2018
@ghost ghost added the review label Feb 18, 2018
@gatsbybot
Copy link
Collaborator

Deploy preview for gatsbygram ready!

Built with commit 5a8f5d0

https://deploy-preview-4122--gatsbygram.netlify.com

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants