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

Fix #6618: sitemap urls generated without slash #6658

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

andremralves
Copy link
Contributor

@andremralves andremralves commented Mar 25, 2023

Changes

closes #6618

I added a condition to check whether the base path has a trailing slash or not (when the base path and page path are concatenated) and add the trailing slash if it's missing.

I tested using the @example/blog with this configuration:

export default defineConfig({
	site: 'https://example.com',
	integrations: [mdx(), sitemap()],
	base: '/base',
	trailingSlash: 'never',
});

Before:

  • sitemap-index.xml
<?xml version="1.0" encoding="UTF-8"?><sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9"><sitemap><loc>https://example.com/sitemap-0.xml</loc></sitemap></sitemapindex>
  • sitemap-0.xml
<?xml version="1.0" encoding="UTF-8"?><urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:news="http://www.google.com/schemas/sitemap-news/0.9" xmlns:xhtml="http://www.w3.org/1999/xhtml" xmlns:image="http://www.google.com/schemas/sitemap-image/1.1" xmlns:video="http://www.google.com/schemas/sitemap-video/1.1"><url><loc>https://example.com/base</loc></url><url><loc>https://example.com/baseabout</loc></url><url><loc>https://example.com/baseblog</loc></url><url><loc>https://example.com/baseblog/first-post</loc></url><url><loc>https://example.com/baseblog/markdown-style-guide</loc></url><url><loc>https://example.com/baseblog/second-post</loc></url><url><loc>https://example.com/baseblog/third-post</loc></url><url><loc>https://example.com/baseblog/using-mdx</loc></url></urlset>

After:

  • sitemap-index.xml
<?xml version="1.0" encoding="UTF-8"?><sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9"><sitemap><loc>https://example.com/base/sitemap-0.xml</loc></sitemap></sitemapindex>
  • sitemap-0.xml
<?xml version="1.0" encoding="UTF-8"?><urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9" xmlns:news="http://www.google.com/schemas/sitemap-news/0.9" xmlns:xhtml="http://www.w3.org/1999/xhtml" xmlns:image="http://www.google.com/schemas/sitemap-image/1.1" xmlns:video="http://www.google.com/schemas/sitemap-video/1.1"><url><loc>https://example.com/base</loc></url><url><loc>https://example.com/base/about</loc></url><url><loc>https://example.com/base/blog</loc></url><url><loc>https://example.com/base/blog/first-post</loc></url><url><loc>https://example.com/base/blog/markdown-style-guide</loc></url><url><loc>https://example.com/base/blog/second-post</loc></url><url><loc>https://example.com/base/blog/third-post</loc></url><url><loc>https://example.com/base/blog/using-mdx</loc></url></urlset>

Testing

I added two new tests for cases were you have a base URL.

  • One to test base URL + trailingSlash: never
  • And the other to test base URL + trailingSlash: always

Docs

No docs needed, just a bug fix

@changeset-bot
Copy link

changeset-bot bot commented Mar 25, 2023

🦋 Changeset detected

Latest commit: 03039b6

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Mar 25, 2023
@andremralves andremralves changed the title Fix sitemap urls generated without slash #6618 Fix #6618: sitemap urls generated without slash Mar 25, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

One tiny nit, but this looks great! Nice tests.

packages/integrations/sitemap/src/index.ts Outdated Show resolved Hide resolved
@43081j
Copy link
Contributor

43081j commented Mar 27, 2023

If we can depend on the node stdlib here, could we just use path.join rather than concatenating strings manually?

More strict comparison

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@andremralves
Copy link
Contributor Author

@43081j I thought about it, but I didn't see they using path.join anywhere in the code base and I would change some code that was already done, so I decided to just use simple concatenation in this case.

@andremralves andremralves requested a review from bluwy April 3, 2023 11:58
@bluwy bluwy merged commit 1ec1df1 into withastro:main Apr 4, 2023
@bluwy
Copy link
Member

bluwy commented Apr 4, 2023

Thanks!

@astrobot-houston astrobot-houston mentioned this pull request Apr 4, 2023
@andremralves andremralves deleted the fix/sitemap-urls-with-base-path branch April 4, 2023 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem generating a sitemap
3 participants