Skip to content

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

Merged
LekoArts merged 13 commits intogatsbyjs:masterfrom
moonmeister:fix/path-bug
May 17, 2021
Merged

fix(gatsby-plugin-sitemap): Sitemap path bug#31184
LekoArts merged 13 commits intogatsbyjs:masterfrom
moonmeister:fix/path-bug

Conversation

@moonmeister
Copy link
Copy Markdown
Contributor

@moonmeister moonmeister commented May 2, 2021

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
Copy Markdown
Contributor Author

moonmeister commented May 2, 2021

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

@moonmeister
Copy link
Copy Markdown
Contributor Author

moonmeister commented May 2, 2021

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
Copy Markdown
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
Copy Markdown
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 added 3 commits May 6, 2021 18:09
@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

pedrolamas
pedrolamas reviewed May 11, 2021
View reviewed changes
@moonmeister
Copy link
Copy Markdown
Contributor Author

moonmeister commented May 13, 2021

I believe we can merge this now.

LekoArts
LekoArts approved these changes May 17, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <mathews.kyle+gatsbybot@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@pedrolamas pedrolamas pedrolamas left review comments

@LekoArts LekoArts LekoArts approved these changes

Reviewers whose approvals may not affect merge requirements

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