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): tracking PR #29140

Closed
wants to merge 2 commits into from
Closed

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Jan 22, 2021

Description

This is tracking branch for moving GATSBY_EXPERIMENTAL_PAGE_BUILD_ON_DATA_CHANGES from experimental to stable

Changes are being plucked from this massive PR into smaller ones:

Already merged:

Ready to review (they are serial - each next one builds on previous and target previous PR branch, so those should be merged in order):

Future PRs:

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 22, 2021
@pieh pieh added topic: scaling builds and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 22, 2021
Comment on lines 50 to 61

- To enable this build option you will need to set an environment variable, which requires access to do so in your build environment.

- This feature is not available with `gatsby develop`.

- You should not try to use this flag alongside Incremental Builds in Gatsby Cloud, as it uses a different process and may conflict with it.

- You will need to persist the `.cache` and `public` directories between builds. This allows for comparisons and reuse of previously built files. If `.cache` directory was not persisted then a full build will be triggered. If `public` directory was not persisted then you might experience failing builds or builds that are missing certain assets.

- Any code changes (templates, components, source handling, new plugins etc) will prompt the creation of a new webpack compilation hash and trigger a full build.

Note: When using the `GATSBY_EXPERIMENTAL_PAGE_BUILD_ON_DATA_CHANGES` flag it is important to do so consistently when building your project. Otherwise, the cache will be cleared and the necessary data for comparison will no longer be available, removing the ability to check for incremental data changes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those changes will need to be extracted to separate PR and only applied when stable version including code changes is released

Comment on lines 41 to 43
if (htmlFile.isDeleted || !state.pages.has(path)) {
// FIXME: checking pages state here because pages are not persisted
// and because of that `isDeleted` might not be set ...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pieh pieh force-pushed the track-html-writes branch 4 times, most recently from 72eddde to 7fdf75f Compare January 27, 2021 11:12
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

Looking 👍

left a few comments

@@ -77,6 +80,20 @@ const getPageData = pagePath => {
}
}

// TODO: verify if memoization is safe given that results can change
const getStaticQueryResult = memoize(hash => {
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be safe for single runs but not in inc builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's the actual thing to verify ;) depends if ssr bundle is "reloaded" or just kept in memory

@@ -152,6 +162,12 @@ const renderHTMLQueue = async (
paths: pageSegment,
})

// is this code path used for dev ssr?
store.dispatch({
Copy link
Contributor

Choose a reason for hiding this comment

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

no it has it's own long-running worker which it sends calls to

const htmlString = await worker.renderHTML({

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good, but I will add test case for it with DEV_SSR enabled to make sure of that (+ shield from potential regressions in future)

@pieh pieh added the breaking change If implemented, this proposed work would break functionality for older versions of Gatsby label Jan 29, 2021
@pieh pieh force-pushed the track-html-writes branch 2 times, most recently from 91ff577 to 2a15b3d Compare February 8, 2021 12:05
@pieh pieh changed the base branch from master to no-fs-in-ssr-no-static-queries-in-ssr-bundle February 12, 2021 00:17
@pieh pieh changed the title refactor(conditional-page-build): explicitly persist status of generated html files refactor(conditional-page-build): tracking PR Feb 12, 2021
@pieh pieh changed the base branch from no-fs-in-ssr-no-static-queries-in-ssr-bundle to master February 12, 2021 12:56
@pieh pieh added the topic: build Related to the Gatsby build process label Feb 12, 2021
@pieh pieh force-pushed the track-html-writes branch 9 times, most recently from 5efa4ae to 4744d83 Compare February 17, 2021 09:03
@pieh
Copy link
Contributor Author

pieh commented Feb 21, 2021

This tracking PR served it's purpose and all that actually ment to be tracked with this PR was merged, so it's to close this one

@pieh pieh closed this Feb 21, 2021
@pieh pieh mentioned this pull request Mar 13, 2021
@LekoArts LekoArts deleted the track-html-writes branch November 1, 2021 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change If implemented, this proposed work would break functionality for older versions of Gatsby topic: build Related to the Gatsby build process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants