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

fix(gatsby-plugin-sharp): use actions provided, don't assume Gatsby module location. #9986

Merged
merged 3 commits into from
Nov 21, 2018

Conversation

martin-css
Copy link
Contributor

@martin-css martin-css commented Nov 16, 2018

The gatsby module resolve by require might not be the gatsby module that is executing. This can occur in mono-repo configurations.

This will prevent the Redux store from receiving the job actions and the bootstrap process will complete without waiting for all images to be processed. This can cause the build to finish without all images being generated.

Fixes #9984.

@pieh
Copy link
Contributor

pieh commented Nov 16, 2018

This is a bit problematic: gatsby-plugin-sharp isn't technically a gatsby plugin (it's utility library) - so it wasn't actually needed to put it in gatsby-config plugin section, so this will break when people don't have in gatsby-config.

We should pass actions that are needed to gatsby-plugin-sharp functions (similiar to how reporter and cache is passed). And, at least for now, have fallback to current behaviour if they are not.

@martin-css
Copy link
Contributor Author

I'm fairly new to Gatsby (started with V2) so I'm not aware of all the history and what could construe a breaking change. All the V2 documentation I remember reading online certainly always instructs that gatsby-plugin-sharp be placed in the plugins section of gatsby-config, for example, gatsby-transformer-sharp.

However, I've amended the pull request to keep the existing functionality in case any user hasn't added to gatsby-config. If gatsby-plugin-sharp is present then it will use the actions passed to the plugin.

Longer term it might be worth refactoring how gatsby-plugin-sharp obtains the bound actions. However, this is a much bigger task and would affect all plugins that depend on gatsby-plugin-sharp, including 3rd party plugins.

This should hopefully now be a non-breaking change that will fix using Gatsby in a mono-repo - as long as plugin is present in gatsby-config as most documentation suggests it should be.

@martin-css martin-css changed the title Use actions provided, don't assume Gatsby module location. Fixes #9984. Use actions provided, don't assume Gatsby module location. Nov 16, 2018
@pieh pieh changed the title Use actions provided, don't assume Gatsby module location. fix(gatsby-plugin-sharp): use actions provided, don't assume Gatsby module location. Nov 21, 2018
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Thanks @martin-css!

@pieh pieh merged commit 54f6a85 into gatsbyjs:master Nov 21, 2018
@gatsbot
Copy link

gatsbot bot commented Nov 21, 2018

Holy buckets, @martin-css — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https:/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

@pieh
Copy link
Contributor

pieh commented Nov 21, 2018

Longer term it might be worth refactoring how gatsby-plugin-sharp obtains the bound actions. However, this is a much bigger task and would affect all plugins that depend on gatsby-plugin-sharp, including 3rd party plugins.

This should hopefully now be a non-breaking change that will fix using Gatsby in a mono-repo - as long as plugin is present in gatsby-config as most documentation suggests it should be.

Yeah, we will probably want to change it later on, but this will work for now

@pieh
Copy link
Contributor

pieh commented Nov 21, 2018

Published [email protected] with those changes

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…odule location. (gatsbyjs#9986)

The gatsby module resolve by require might not be the gatsby module that is executing. This can occur in mono-repo configurations.

This will prevent the Redux store from receiving the job actions and the bootstrap process will complete without waiting for all images to be processed. This can cause the build to finish without all images being generated.

Fixes gatsbyjs#9984.
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.

[gatsby-plugin-sharp] Requires incorrect gatsby module in mono-repo
2 participants