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

refactor(conditional-page-build): explicitly persist status of generated html files #29473

Merged
merged 11 commits into from
Feb 16, 2021

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Feb 12, 2021

Description

This moves GATSBY_EXPERIMENTAL_PAGE_BUILD_ON_DATA_CHANGES page query result changes and webpack compilation hash changes tracking from "transient" to more explicit tracking.

It achieves few things:

  • it removes some restrictions of it (as indicated by conditional-page-builds.md changes):
    • doesn't require users to consistently use the flag (so no cache clearing if there are some builds that use the flag and some that do not)
    • allow mixing with gatsby develop sessions in-between gatsby builds
  • it removes a lot of conditional code for this feature and make it share a whole lot more with default (build all) mode. Only conditional code right now is:
    • deciding which html files to generate
    • whether all html files get deleted ahead of time
    • handling of GATSBY_EXPERIMENTAL_PAGE_BUILD_ON_DATA_CHANGES specific cli switches (--log-pages and --write-to-file)

PR builds on #29396

[ch23311]
[ch23313]
[ch23488]
[ch23535]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 12, 2021
@pieh pieh added topic: SSG* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 12, 2021
@pieh pieh force-pushed the refactor-current-cpb branch 2 times, most recently from a617fb8 to ed40a7d Compare February 12, 2021 12:58
@pieh pieh force-pushed the refactor-current-cpb branch 4 times, most recently from 05ce91f to 5ea1e47 Compare February 12, 2021 14:41
@pieh pieh marked this pull request as ready for review February 12, 2021 15:13
@pieh pieh marked this pull request as draft February 12, 2021 15:17
@pieh pieh marked this pull request as ready for review February 12, 2021 19:21
@pieh pieh added the topic: build Related to the Gatsby build process label Feb 14, 2021
@pieh pieh force-pushed the refactor-current-cpb branch 3 times, most recently from d6a05c4 to d9f226d Compare February 14, 2021 17:56
@pieh pieh force-pushed the no-fs-in-ssr-no-static-queries-in-ssr-bundle branch from 0c83fdc to aac9d30 Compare February 15, 2021 11:50
Base automatically changed from no-fs-in-ssr-no-static-queries-in-ssr-bundle to master February 15, 2021 17:28
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Have a couple of questions inline, but nothing blocking.


const FLAG_DIRTY_CLEARED_CACHE = 0b0000001
const FLAG_DIRTY_NEW_PAGE = 0b0000010
const FLAG_DIRTY_PAGE_QUERY_CHANGED = 0b0000100 // TODO: this need to be PAGE_DATA and not PAGE_QUERY, but requires some shuffling
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Also noticed that PAGE_QUERY is a bit confusing name for what it actually is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#29498 is one that addresses TODO here. This PR is largely 1 to 1 refactor of current stuff and introduction of html reducer and tried to avoid making more changes than just to keep what we currently track so far

Comment on lines +27 to +34
// we can't just clear the cache here - we want to keep track of pages, so we mark them all as "deleted"
// if they are recreated "isDeleted" flag will be removed
state.browserCompilationHash = ``
state.trackedHtmlFiles.forEach(htmlFile => {
htmlFile.isDeleted = true
// there was a change somewhere, so just in case we mark those files are dirty as well
htmlFile.dirty |= FLAG_DIRTY_CLEARED_CACHE
})
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the downside of just clearing the cache here? Is it only that we will re-build all HTML files? Or also that there could be some stale files left as a result?

I am not sure if it makes sense to have special handling for this case as it makes things more complicated in general. Ideally, files should be the derived artifact of our state but not the other way around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With code as-is - this is to remove .html / page-data files that are not recreated later on.

Ideally, files should be the derived artifact of our state but not the other way around?

I'm not sure if this is the case of the other way around - I don't exactly use individual files and work backward to create state from them - instead this is

We will blow away our state, but there are will be some files created from state we had before and we will need to keep part of that state to know what we need to clean up. If we want to start completely clean, we will need to do expensive public deletion (or traverse public dir and delete some files)

Not the hill I will die on, but we do have opportunity to use previous state to do targetted deletions instead of brute force deletions ;)

@pieh pieh merged commit 2b7d230 into master Feb 16, 2021
@pieh pieh deleted the refactor-current-cpb branch February 16, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: build Related to the Gatsby build process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants