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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby): handle case of removing trailing slash in inc builds #29953

Merged
merged 7 commits into from
Mar 4, 2021
Merged

fix(gatsby): handle case of removing trailing slash in inc builds #29953

merged 7 commits into from
Mar 4, 2021

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Mar 3, 2021

Description

This is to handle cases like:

exports.onCreatePage = ({ page, actions }) => {
  if (someConditionThatCheckPagePath) {
    actions.deletePage(page)
    actions.createPage({
      ...page,
      path: slightlyChangedPagePath,
    })
  }
}

Used in plugins like gatsby-plugin-remove-trailing-slashes etc.

For now this add failing test scenario, will add a fix for it in next commit

Related Issues

Fixes #29946

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 3, 2021
@pieh pieh added topic: build Related to the Gatsby build process and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 3, 2021
@pieh pieh force-pushed the ib-handle-remove-trailing-slashes-case branch from 5948f4e to 357d094 Compare March 3, 2021 11:30
pieh added 3 commits March 3, 2021 16:18
@pieh
add one more edge case to tests
8bb7ed0
@pieh
add more assertions
09cfe04
@pieh
fix(gatsby): [incremental builds] handle case of page path changing d…
6bbefcb 
…uring 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
wardpeet
wardpeet previously approved these changes Mar 4, 2021
View reviewed changes
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.

Code looks good, would it make sense to add unit tests to packages/gatsby/src/commands/build-utils.ts to test the logic instead of only relying on e2e tests

packages/gatsby/src/commands/build-utils.ts Outdated Show resolved Hide resolved
@pieh pieh marked this pull request as ready for review March 4, 2021 13:44
vladar
vladar approved these changes Mar 4, 2021
View reviewed changes
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.

Thank you 👍

@vladar vladar merged commit 7462030 into master Mar 4, 2021
@vladar vladar deleted the ib-handle-remove-trailing-slashes-case branch March 4, 2021 16:58
@vladar vladar added this to To cherry-pick in V3 Release Hotfixes via automation Mar 4, 2021
@gatsbybot gatsbybot mentioned this pull request Mar 4, 2021
Merged
vladar pushed a commit that referenced this pull request Mar 4, 2021
@pieh @vladar
fix(gatsby): handle case of removing trailing slash in inc builds (#2…
56bee9b 
…9953)

* 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

(cherry picked from commit 7462030)
@vladar vladar moved this from To cherry-pick to Backport PR opened in V3 Release Hotfixes Mar 4, 2021
vladar pushed a commit that referenced this pull request Mar 4, 2021
@pieh
fix(gatsby): handle case of removing trailing slash in inc builds (#2…
d050050 
…9953) (#30001)

* 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

(cherry picked from commit 7462030)

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
@vladar vladar moved this from Backport PR opened to Backported in V3 Release Hotfixes Mar 4, 2021
@vladar
Copy link
Contributor

vladar commented Mar 4, 2021

Published in gatsby@3.0.2

@vladar vladar moved this from Backported to Archived in V3 Release Hotfixes Mar 5, 2021
@dependabot dependabot bot mentioned this pull request Mar 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vladar vladar vladar approved these changes

@wardpeet wardpeet wardpeet left review comments

Assignees
No one assigned
Labels
topic: build Related to the Gatsby build process
Projects
No open projects
V3 Release Hotfixes
Published
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pages not generated in Gatsby v3 build
3 participants
@pieh @vladar @wardpeet