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-plugin-sitemap): Sitemap path bug #31184

Merged
merged 13 commits into from
May 17, 2021
Merged

fix(gatsby-plugin-sitemap): Sitemap path bug #31184

merged 13 commits into from
May 17, 2021

Conversation

moonmeister
Copy link
Contributor

Description

Fixes the bug where sitemaps were being written in a sub-directory but the sitemap index didn't contain that sub-directory in the path.

Related Issues

Fixes #31167

@moonmeister moonmeister added type: bug An issue or pull request relating to a bug in Gatsby topic: sitemap labels May 2, 2021
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 2, 2021
@moonmeister
Copy link
Contributor Author

There's probably still a bug with pathPrefix. Confirming that now and will write tests for all this.

@moonmeister
Copy link
Contributor Author

Should be good to go

@moonmeister moonmeister mentioned this pull request May 2, 2021
Merged
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 3, 2021
@LekoArts
Copy link
Contributor

LekoArts commented May 3, 2021

The test currently fails at this:

  ● gatsby-plugin-sitemap Node API › should succeed with default options

    expect(received).toEqual(expected) // deep equality

    Expected: "public/"
    Received: "public\\"

      56 |       sourceData,
      57 |     } = sitemap.simpleSitemapAndIndex.mock.calls[0][0]
    > 58 |     expect(destinationDir).toEqual(`public/`)
         |                            ^
      59 |     expect(sourceData).toMatchSnapshot()
      60 |   })
      61 |

@moonmeister
Copy link
Contributor Author

moonmeister commented May 3, 2021 •
edited
Loading

Thanks, I missed that, I'll look into it.

@rudouglas rudouglas mentioned this pull request May 4, 2021
Closed
@moonmeister
Merge branch 'master' into fix/path-bug
fc936ce 
* master: (45 commits)
  chore(release): Publish next pre-minor
  fix(gatsby-source-shopify): fix linting (gatsbyjs#31291)
  fix(deps): update minor and patch for gatsby-plugin-preact (gatsbyjs#31169)
  chore: add gatsby-plugin-gatsby-cloud to renovate
  chore: update renovatebot config to support more packages (gatsbyjs#31289)
  chore(deps): update dependency @types/semver to ^7.3.5 (gatsbyjs#31148)
  fix(deps): update minor and patch for gatsby-plugin-manifest (gatsbyjs#31160)
  fix(deps): update minor and patch for gatsby-remark-copy-linked-files (gatsbyjs#31163)
  fix(deps): update dependency mini-css-extract-plugin to v1.6.0 (gatsbyjs#31158)
  chore(deps): update dependency @testing-library/react to ^11.2.6 (gatsbyjs#31168)
  docs(gatsby-source-shopify): Updates Shopify README with new plugin info (gatsbyjs#31287)
  chore: run yarn deduplicate (gatsbyjs#31285)
  docs(gatsby-plugin-image): Add docs for customizing default options (gatsbyjs#30344)
  fix(gatsby-plugin-image): print error details (gatsbyjs#30417)
  chore(docs): Update "Adding Search with Algolia" guide (gatsbyjs#29460)
  chore(docs): Update MDX frontmatter for programmatic pages (gatsbyjs#29798)
  docs: Add image plugin architecture doc (gatsbyjs#31096)
  perf(gatsby): use fastq instead of better-queue + refactor (gatsbyjs#31269)
  feat(gatsby-plugin-image): Export ImageDataLike type (gatsbyjs#30590)
  fix(gatsby): update plugin api types (gatsbyjs#30819)
  ...
@moonmeister
fix: windows path
ec79401
@moonmeister
Merge branch 'fix/path-bug' of github.com:moonmeister/gatsby into fix…
679e274 
…/path-bug
@moonmeister
Copy link
Contributor Author

moonmeister commented May 7, 2021 •
edited
Loading

okay, windows test should be good now.

Update: well, not failing due to these changes or the sitemap plugin. LGTM

@LekoArts
Copy link
Contributor

LekoArts commented May 7, 2021

Mhh, but now the PR has a breaking change. It changes the default output and people might already rely on it being in sitemap folder. The double slash bug you mentioned in ekalinin/sitemap.js#359 was fixed with v7, so can we keep the same behavior for now and fix the bug that e.g. people like newrelic/docs-website#2103 see when they use / as path?

@moonmeister
Copy link
Contributor Author

moonmeister commented May 7, 2021 •
edited
Loading

I was thinking it wouldn't be breaking but you're right. I'll revert those changes.

@moonmeister
Copy link
Contributor Author

I believe we can merge this now.

LekoArts
LekoArts approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Thanks!

@LekoArts LekoArts merged commit d95b258 into gatsbyjs:master May 17, 2021
@LekoArts LekoArts mentioned this pull request May 18, 2021
Closed
@vladar
Copy link
Contributor

vladar commented May 19, 2021

This PR contains a major bump for one of the main dependencies: sitemap. I think we should not publish it as a patch version. So it will be published in the next minor 4.2.0 on the 25th of May 2021.

In the meantime anyone who needs it can use gatsby-plugin-sitemap@next

@moonmeister
Copy link
Contributor Author

moonmeister commented May 20, 2021 •
edited
Loading

This PR contains a major bump for one of the main dependencies: sitemap. I think we should not publish it as a patch version. So it will be published in the next minor 4.2.0 on the 25th of May 2021.

In the meantime anyone who needs it can use gatsby-plugin-sitemap@next

The major bump doesn't break anything but it does fix a clinical bug. It'd be nice if we could release this ASAP

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
@gatsbybot
fix(gatsby-plugin-sitemap): Sitemap path bug (gatsbyjs#31184)
7dd08db 
Co-authored-by: gatsbybot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrolamas pedrolamas pedrolamas left review comments

@LekoArts LekoArts LekoArts approved these changes

Assignees
No one assigned
Labels
type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gatsby-plugin-sitemap: incorrect sitemap path in index sitemap
4 participants
@moonmeister @LekoArts @vladar @pedrolamas