From 7462030b9f3c6ad028a2469b850e3477b4f0b954 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Thu, 4 Mar 2021 17:58:36 +0100 Subject: [PATCH] fix(gatsby): handle case of removing trailing slash in inc builds (#29953) * add test case * add one more edge case to tests * add more assertions * fix(gatsby): [incremental builds] handle case of page path changing during or between builds that wouldn't result in change of artifact filenames this is to cover for cases like `gatsby-plugin-remove-trailing-slashes` that change page path during the build or case when page path might be created from some cms content and trailing slash being added or removed there * make normalizePagePath terser * initial setup for calcDirtyHtmlFiles unit tests * flesh out tests --- .../artifacts/__tests__/index.js | 11 +- integration-tests/artifacts/gatsby-node.js | 26 ++ ...e-that-will-have-trailing-slash-removed.js | 5 + .../src/commands/__tests__/build-utils.ts | 316 ++++++++++++++++++ packages/gatsby/src/commands/build-html.ts | 13 +- packages/gatsby/src/commands/build-utils.ts | 86 ++++- packages/gatsby/src/redux/reducers/html.ts | 11 + packages/gatsby/src/redux/types.ts | 6 + 8 files changed, 463 insertions(+), 11 deletions(-) create mode 100644 integration-tests/artifacts/src/pages/page-that-will-have-trailing-slash-removed.js create mode 100644 packages/gatsby/src/commands/__tests__/build-utils.ts diff --git a/integration-tests/artifacts/__tests__/index.js b/integration-tests/artifacts/__tests__/index.js index 417765a0bc5c2..69a7429fc7bfa 100644 --- a/integration-tests/artifacts/__tests__/index.js +++ b/integration-tests/artifacts/__tests__/index.js @@ -369,7 +369,12 @@ describe(`First run (baseline)`, () => { }) }) - const expectedPages = [`stale-pages/stable`, `stale-pages/only-in-first`] + const expectedPages = [ + `stale-pages/stable`, + `stale-pages/only-in-first`, + `page-that-will-have-trailing-slash-removed`, + `/stale-pages/sometimes-i-have-trailing-slash-sometimes-i-dont`, + ] const unexpectedPages = [`stale-pages/only-not-in-first`] describe(`html files`, () => { @@ -460,6 +465,7 @@ describe(`Second run (different pages created, data changed)`, () => { `/page-query-dynamic-2/`, `/static-query-result-tracking/should-invalidate/`, `/page-query-template-change/`, + `/stale-pages/sometimes-i-have-trailing-slash-sometimes-i-dont/`, ] const expectedPagesToRemainFromPreviousBuild = [ @@ -468,6 +474,7 @@ describe(`Second run (different pages created, data changed)`, () => { `/page-query-changing-but-not-invalidating-html/`, `/static-query-result-tracking/stable/`, `/static-query-result-tracking/rerun-query-but-dont-recreate-html/`, + `/page-that-will-have-trailing-slash-removed`, ] const expectedPages = [ @@ -542,6 +549,8 @@ describe(`Third run (js change, all pages are recreated)`, () => { const expectedPages = [ `/stale-pages/only-not-in-first`, `/page-query-dynamic-3/`, + `/page-that-will-have-trailing-slash-removed`, + `/stale-pages/sometimes-i-have-trailing-slash-sometimes-i-dont`, ] const unexpectedPages = [ diff --git a/integration-tests/artifacts/gatsby-node.js b/integration-tests/artifacts/gatsby-node.js index ca61039d8d646..be1fb7fed430a 100644 --- a/integration-tests/artifacts/gatsby-node.js +++ b/integration-tests/artifacts/gatsby-node.js @@ -31,6 +31,7 @@ exports.onPreInit = ({ emitter }) => { } let previouslyCreatedNodes = new Map() +let didRemoveTrailingSlashForTestedPage = false exports.sourceNodes = ({ actions, @@ -145,6 +146,12 @@ exports.createPages = async ({ actions, graphql }) => { createPageHelper(`only-not-in-first`) } + createPageHelper( + `sometimes-i-have-trailing-slash-sometimes-i-dont${ + runNumber % 2 === 0 ? `/` : `` + }` + ) + const { data } = await graphql(` { allDepPageQuery { @@ -181,6 +188,13 @@ exports.onPreBuild = () => { let counter = 1 exports.onPostBuild = async ({ graphql }) => { console.log(`[test] onPostBuild`) + + if (!didRemoveTrailingSlashForTestedPage) { + throw new Error( + `Test setup failed - didn't remove trailing slash for /pages-that-will-have-trailing-slash-removed/ page` + ) + } + const { data } = await graphql(` { allSitePage(filter: { path: { ne: "/dev-404-page/" } }) { @@ -206,3 +220,15 @@ exports.onPostBuild = async ({ graphql }) => { } ) } + +// simulating "gatsby-plugin-remove-trailing-slashes" scenario +exports.onCreatePage = ({ page, actions }) => { + if (page.path === `/page-that-will-have-trailing-slash-removed/`) { + actions.deletePage(page) + actions.createPage({ + ...page, + path: `/page-that-will-have-trailing-slash-removed`, + }) + didRemoveTrailingSlashForTestedPage = true + } +} diff --git a/integration-tests/artifacts/src/pages/page-that-will-have-trailing-slash-removed.js b/integration-tests/artifacts/src/pages/page-that-will-have-trailing-slash-removed.js new file mode 100644 index 0000000000000..fe852c484fba6 --- /dev/null +++ b/integration-tests/artifacts/src/pages/page-that-will-have-trailing-slash-removed.js @@ -0,0 +1,5 @@ +import * as React from "react" + +export default function NoTrailingSlashPage({ path, pathname }) { + return
I don't have trailing slash
+} diff --git a/packages/gatsby/src/commands/__tests__/build-utils.ts b/packages/gatsby/src/commands/__tests__/build-utils.ts new file mode 100644 index 0000000000000..8ba4fcf0ac619 --- /dev/null +++ b/packages/gatsby/src/commands/__tests__/build-utils.ts @@ -0,0 +1,316 @@ +import { + IGatsbyState, + IGatsbyPage, + IHtmlFileState, + IStaticQueryResultState, +} from "../../redux/types" +import { calcDirtyHtmlFiles } from "../build-utils" + +interface IMinimalStateSliceForTest { + html: IGatsbyState["html"] + pages: IGatsbyState["pages"] +} + +describe(`calcDirtyHtmlFiles`, () => { + function generateStateToTestHelper( + pages: Record< + string, + { + dirty: number + removedOrDeleted?: "deleted" | "not-recreated" + } + > + ): IGatsbyState { + const state: IMinimalStateSliceForTest = { + pages: new Map(), + html: { + browserCompilationHash: `a-hash`, + ssrCompilationHash: `a-hash`, + trackedHtmlFiles: new Map(), + unsafeBuiltinWasUsedInSSR: false, + trackedStaticQueryResults: new Map(), + }, + } + + for (const pagePath in pages) { + const page = pages[pagePath] + + if (page.removedOrDeleted !== `not-recreated`) { + state.pages.set(pagePath, { + component: `/foo`, + componentPath: `/foo`, + componentChunkName: `foo`, + context: {}, + internalComponentName: `foo`, + isCreatedByStatefulCreatePages: false, + path: pagePath, + matchPath: undefined, + pluginCreatorId: `foo`, + // eslint-disable-next-line @typescript-eslint/naming-convention + pluginCreator___NODE: `foo`, + updatedAt: 1, + }) + } + + state.html.trackedHtmlFiles.set(pagePath, { + dirty: page.dirty, + pageDataHash: `a-hash`, + isDeleted: page.removedOrDeleted === `deleted`, + }) + } + + return state as IGatsbyState + } + + it(`nothing changed`, () => { + const state = generateStateToTestHelper({ + // page not dirty - we can reuse, so shouldn't be regenerated, deleted or cleaned up + "/to-reuse/": { + dirty: 0, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + // as nothing changed nothing should be regenerated, deleted or cleaned up + expect(results.toRegenerate.sort()).toEqual([]) + expect(results.toDelete.sort()).toEqual([]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([]) + }) + + it(`content for few pages changed`, () => { + const state = generateStateToTestHelper({ + // pages were marked as dirty for whatever reason + "/to-regenerate/": { + dirty: 42, + }, + "/to-regenerate/nested/": { + dirty: 42, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + // as pages are marked as dirty, artifacts for those should be (re)generated + expect(results.toRegenerate.sort()).toEqual([ + `/to-regenerate/`, + `/to-regenerate/nested/`, + ]) + expect(results.toDelete.sort()).toEqual([]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([]) + }) + + it(`few pages were deleted`, () => { + const state = generateStateToTestHelper({ + // pages were deleted with `deletePage` action + "/deleted/": { + dirty: 0, + removedOrDeleted: `deleted`, + }, + "/deleted/nested/": { + dirty: 42, + removedOrDeleted: `deleted`, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + // as pages are marked as deleted, artifacts for those should be deleted + expect(results.toRegenerate.sort()).toEqual([]) + expect(results.toDelete.sort()).toEqual([`/deleted/`, `/deleted/nested/`]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([]) + }) + + it(`few pages were not re-created`, () => { + const state = generateStateToTestHelper({ + // pages are tracked, but were not recreated + "/not-recreated/": { + dirty: 0, + removedOrDeleted: `not-recreated`, + }, + "/not-recreated/nested/": { + dirty: 0, + removedOrDeleted: `not-recreated`, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + // as pages are not recreated, artifacts for those should be deleted + expect(results.toRegenerate.sort()).toEqual([]) + expect(results.toDelete.sort()).toEqual([ + `/not-recreated/`, + `/not-recreated/nested/`, + ]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([]) + }) + + describe(`onCreatePage + deletePage + createPage that change path of a page (remove trailing slash)`, () => { + it(`page is dirty`, () => { + const state = generateStateToTestHelper({ + // page was created, then deleted and similar page with slightly different path was created for it + // both page paths would result in same artifacts + "/remove-trailing-slashes-dirty/": { + dirty: 0, + removedOrDeleted: `deleted`, + }, + "/remove-trailing-slashes-dirty": { + dirty: 42, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + // as pages would generate artifacts with same filenames - we expect artifact for + // deleted page NOT to be deleted, but instead just tracking state clean up + // and regeneration of new page with adjusted path + expect(results.toRegenerate.sort()).toEqual([ + `/remove-trailing-slashes-dirty`, + ]) + expect(results.toDelete.sort()).toEqual([]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([ + `/remove-trailing-slashes-dirty/`, + ]) + }) + + it(`page is NOT dirty`, () => { + const state = generateStateToTestHelper({ + // page was created, then deleted and similar page with slightly different path was created for it + // both page paths would result in same artifacts + "/remove-trailing-slashes-not-dirty/": { + dirty: 0, + removedOrDeleted: `deleted`, + }, + "/remove-trailing-slashes-not-dirty": { + dirty: 0, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + // as pages would generate artifacts with same filenames - we expect artifact for + // deleted page NOT to be deleted, but instead just tracking state clean up + // adjusted page is not marked as dirty so it shouldn't regenerate () + expect(results.toRegenerate.sort()).toEqual([]) + expect(results.toDelete.sort()).toEqual([]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([ + `/remove-trailing-slashes-not-dirty/`, + ]) + }) + }) + + it(`slash was removed between builds (without onCreatePage + deletePage combination)`, () => { + const state = generateStateToTestHelper({ + // page was created in previous build, but not recreated in current one + // instead page with slightly different path is created + // both page paths would result in same artifacts + "/slash-removed-without-onCreatePage/": { + dirty: 0, + removedOrDeleted: `not-recreated`, + }, + "/slash-removed-without-onCreatePage": { + dirty: 1, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + expect(results.toRegenerate.sort()).toEqual([ + `/slash-removed-without-onCreatePage`, + ]) + expect(results.toDelete.sort()).toEqual([]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([ + `/slash-removed-without-onCreatePage/`, + ]) + }) + + // cases above are to be able to pinpoint exact failure, kitchen sink case is to test all of above in one go + // and make sure that various conditions mixed together are handled correctly + it(`kitchen sink`, () => { + const state = generateStateToTestHelper({ + // page not dirty - we can reuse, so shouldn't be regenerated, deleted or cleaned up + "/to-reuse/": { + dirty: 0, + }, + + // pages were marked as dirty for whatever reason + "/to-regenerate/": { + dirty: 42, + }, + "/to-regenerate/nested/": { + dirty: 42, + }, + + // pages were deleted with `deletePage` action + "/deleted/": { + dirty: 0, + removedOrDeleted: `deleted`, + }, + "/deleted/nested/": { + dirty: 42, + removedOrDeleted: `deleted`, + }, + + // pages are tracked, but were not recreated + "/not-recreated/": { + dirty: 0, + removedOrDeleted: `not-recreated`, + }, + "/not-recreated/nested/": { + dirty: 0, + removedOrDeleted: `not-recreated`, + }, + + // page was created, then deleted and similar page with slightly different path was created for it + // both page paths would result in same artifacts + "/remove-trailing-slashes-dirty/": { + dirty: 0, + removedOrDeleted: `deleted`, + }, + "/remove-trailing-slashes-dirty": { + dirty: 42, + }, + + // page was created, then deleted and similar page with slightly different path was created for it + // both page paths would result in same artifacts + "/remove-trailing-slashes-not-dirty/": { + dirty: 0, + removedOrDeleted: `deleted`, + }, + "/remove-trailing-slashes-not-dirty": { + dirty: 0, + }, + + // page was created in previous build, but not recreated in current one + // instead page with slightly different path is created + // both page paths would result in same artifacts + "/slash-removed-without-onCreatePage/": { + dirty: 0, + removedOrDeleted: `not-recreated`, + }, + "/slash-removed-without-onCreatePage": { + dirty: 1, + }, + }) + + const results = calcDirtyHtmlFiles(state) + + expect(results.toRegenerate.sort()).toEqual([ + `/remove-trailing-slashes-dirty`, + `/slash-removed-without-onCreatePage`, + `/to-regenerate/`, + `/to-regenerate/nested/`, + ]) + expect(results.toDelete.sort()).toEqual([ + `/deleted/`, + `/deleted/nested/`, + `/not-recreated/`, + `/not-recreated/nested/`, + ]) + expect(Array.from(results.toCleanupFromTrackedState).sort()).toEqual([ + `/remove-trailing-slashes-dirty/`, + `/remove-trailing-slashes-not-dirty/`, + `/slash-removed-without-onCreatePage/`, + ]) + }) +}) diff --git a/packages/gatsby/src/commands/build-html.ts b/packages/gatsby/src/commands/build-html.ts index 1fc0e28a50149..b6d1112de919e 100644 --- a/packages/gatsby/src/commands/build-html.ts +++ b/packages/gatsby/src/commands/build-html.ts @@ -319,9 +319,16 @@ export async function buildHTMLPagesAndDeleteStaleArtifacts({ }> { buildUtils.markHtmlDirtyIfResultOfUsedStaticQueryChanged() - const { toRegenerate, toDelete } = buildUtils.calcDirtyHtmlFiles( - store.getState() - ) + const { + toRegenerate, + toDelete, + toCleanupFromTrackedState, + } = buildUtils.calcDirtyHtmlFiles(store.getState()) + + store.dispatch({ + type: `HTML_TRACKED_PAGES_CLEANUP`, + payload: toCleanupFromTrackedState, + }) if (toRegenerate.length > 0) { const buildHTMLActivityProgress = reporter.createProgress( diff --git a/packages/gatsby/src/commands/build-utils.ts b/packages/gatsby/src/commands/build-utils.ts index c50ac8a25073d..e075aba991f91 100644 --- a/packages/gatsby/src/commands/build-utils.ts +++ b/packages/gatsby/src/commands/build-utils.ts @@ -66,11 +66,80 @@ export const removePageFiles = async ( }) } +function normalizePagePath(path: string): string { + if (path === `/`) { + return `/` + } + return path.endsWith(`/`) ? path.slice(0, -1) : path +} + +type PageGenerationAction = "delete" | "regenerate" | "reuse" +const pageGenerationActionPriority: Record = { + // higher the number, higher the priority + regenerate: 2, + reuse: 1, + delete: 0, +} + export function calcDirtyHtmlFiles( state: IGatsbyState -): { toRegenerate: Array; toDelete: Array } { - const toRegenerate: Array = [] - const toDelete: Array = [] +): { + toRegenerate: Array + toDelete: Array + toCleanupFromTrackedState: Set +} { + const toRegenerate = new Set() + const toDelete = new Set() + const toCleanupFromTrackedState = new Set() + const normalizedPagePathToAction = new Map< + string, + { + actualPath: string + action: PageGenerationAction + } + >() + + /** + * multiple page paths can result in same html and page-data filenames + * so we need to keep that in mind when generating list of pages + * to regenerate and more importantly - to delete (so we don't delete html and page-data file + * when path changes slightly but it would still result in same html and page-data filenames + * for example adding/removing trailing slash between builds or even mid build with plugins + * like `gatsby-plugin-remove-trailing-slashes`) + */ + function markActionForPage(path: string, action: PageGenerationAction): void { + const normalizedPagePath = normalizePagePath(path) + + const previousAction = normalizedPagePathToAction.get(normalizedPagePath) + let overwritePreviousAction = false + if (previousAction) { + const previousActionPriority = + pageGenerationActionPriority[previousAction.action] + const currentActionPriority = pageGenerationActionPriority[action] + + if (currentActionPriority > previousActionPriority) { + overwritePreviousAction = true + toCleanupFromTrackedState.add(previousAction.actualPath) + if (previousAction.action === `delete`) { + // "reuse" or "regenerate" will take over, so we should + // remove path from list of paths to delete + toDelete.delete(previousAction.actualPath) + } + } + } + + if (!previousAction || overwritePreviousAction) { + normalizedPagePathToAction.set(normalizedPagePath, { + actualPath: path, + action, + }) + if (action === `delete`) { + toDelete.add(path) + } else if (action === `regenerate`) { + toRegenerate.add(path) + } + } + } if (state.html.unsafeBuiltinWasUsedInSSR) { reporter.warn( @@ -82,15 +151,18 @@ export function calcDirtyHtmlFiles( 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 ... - toDelete.push(path) + markActionForPage(path, `delete`) } else if (htmlFile.dirty || state.html.unsafeBuiltinWasUsedInSSR) { - toRegenerate.push(path) + markActionForPage(path, `regenerate`) + } else { + markActionForPage(path, `reuse`) } }) return { - toRegenerate, - toDelete, + toRegenerate: Array.from(toRegenerate), + toDelete: Array.from(toDelete), + toCleanupFromTrackedState, } } diff --git a/packages/gatsby/src/redux/reducers/html.ts b/packages/gatsby/src/redux/reducers/html.ts index c1c9d4a3dcf01..f0e55ce8a70ad 100644 --- a/packages/gatsby/src/redux/reducers/html.ts +++ b/packages/gatsby/src/redux/reducers/html.ts @@ -163,6 +163,17 @@ export function htmlReducer( return state } + case `HTML_TRACKED_PAGES_CLEANUP`: { + // this is to cleanup variants of page paths that don't result in artifacts deletion + // but page path should be pruned for cases like page changing path from "/foo" to "/foo/" (or vice versa) + // where produced artifacts filenames are the same and we don't want to delete them after building, + // but we still want to cleanup state here. + for (const path of action.payload) { + state.trackedHtmlFiles.delete(path) + } + return state + } + case `HTML_GENERATED`: { for (const path of action.payload) { const htmlFile = state.trackedHtmlFiles.get(path) diff --git a/packages/gatsby/src/redux/types.ts b/packages/gatsby/src/redux/types.ts index 36a5bfbf0d2be..92b533e1e4d28 100644 --- a/packages/gatsby/src/redux/types.ts +++ b/packages/gatsby/src/redux/types.ts @@ -372,6 +372,7 @@ export type ActionsUnion = | ISetProgramExtensions | IDeletedStalePageDataFiles | IRemovedHtml + | ITrackedHtmlCleanup | IGeneratedHtml | IMarkHtmlDirty | ISSRUsedUnsafeBuiltin @@ -829,6 +830,11 @@ interface IRemovedHtml { payload: string } +interface ITrackedHtmlCleanup { + type: `HTML_TRACKED_PAGES_CLEANUP` + payload: Set +} + interface IGeneratedHtml { type: `HTML_GENERATED` payload: Array