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): notify when Gatsby cache is incomplete #27549

Merged
merged 1 commit into from
Oct 19, 2020

Conversation

vladar
Copy link
Contributor

@vladar vladar commented Oct 19, 2020

Description

This PR adds a warning and wipes the .cache when the public folder is missing but the .cache folder exists.

Why

Builds can break if someone deletes the public folder but not the .cache (with cryptic errors about missing page-data files).

There is one case when doing this still leads to accidentally working builds: when the project only has pages without any queries. This behavior actually relies on our "bug" - at the moment we always re-run page queries without dependencies (or in other words - pages without graphql queries in them).

This skipped test sums up the problem:

// TO-DO: this is known issue - we always rerun queries for pages with no dependencies
// this mean that we will retry to rerun them every time we restart gatsby
it.skip(`rerunning should not run any queries (with restart)`, async () => {

That's exactly what happens in gatsby-admin and our long-term-caching integration tests (we don't have any build-time graphql queries in those and delete the public folder without deleting the .cache). At the moment this "works" but won't work anymore after #27504 lands (which actually fixes this bug).

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 19, 2020
@vladar vladar removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 19, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

LGTM :) this has created issues in the past. I'm sure of it. THank you!

@KyleAMathews
Copy link
Contributor

🙏 this has frustrated a lot of people in the past, thanks!

@vladar vladar added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Oct 19, 2020
@gatsbybot gatsbybot merged commit 2d6a153 into master Oct 19, 2020
@delete-merged-branch delete-merged-branch bot deleted the vladar/inconsistent-cache-warning branch October 19, 2020 15:30
@quantizor
Copy link

I'm a bit disappointed that y'all are doubling down on requiring public instead of keeping the page-data files in .cache and copying them into public during builds. This is an anti-pattern.

@KyleAMathews
Copy link
Contributor

@probablyup this is something we can do in the future but it's a bigger change than simply verifying the state of the public folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants