Skip to content

Conversation

@pieh
Copy link
Contributor

@pieh pieh commented Feb 18, 2018

Please don't merge without review!

In #4076 I added implicit dependency on gatsby-cli/lib/reporter to gatsby-plugin-sharp, which would works fine when installing dependencies with yarn and not with npm.

gatsby package has listed gatsby-cli as dependency and yarn will install both gatsby and gatsby-cli in project node_modules and npm will install gatsby-cli in gatsbys node_modules.

Propably better way would be exporting reporter from gatsby (similar to gatsby-link exported from gatsby in v2), but this would cause another conflict to resolve when merging master into v2 which I'd like to avoid.

@ghost ghost assigned pieh Feb 18, 2018
@ghost ghost added the review label Feb 18, 2018
@KyleAMathews
Copy link
Contributor

Deploy preview for using-glamor failed.

Built with commit af0a9d5

https://app.netlify.com/sites/using-glamor/deploys/5a898b4a295b553260851009

@jquense
Copy link
Contributor

jquense commented Feb 18, 2018

hmm we sohuldn't be relying on the reporter like this generally. The reporter is already injected as an argument for each plugin, which gives us more freedom to group/layout how plugin output is rendered. The right fix for this is to use https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/src/utils/api-runner-node.js#L79 in the plugin from gatsby

@pieh
Copy link
Contributor Author

pieh commented Feb 18, 2018

Right, I've missed it's passed when using node api. I will close this and reopen with proper fix.

This is kind of problematic - gatsby-plugin-sharp doesn't make direct use of api hooks - it export utility functions that other plugins make use of (gatsby-transformer-sharp and gatsby-remark-images from official plugins) so we need to pass reporter from other plugins. But I guess same is done with createRemoteFileNode from gatsby-source-filesystem it require a lot of stuff like that to be passed along remote url.

Should we warn that reporter wasn't passed for 3rd party plugins? :)

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.

3 participants