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

Recommended method can produce invalid sitemap #11869

Closed
ShMcK opened this issue Feb 18, 2019 · 6 comments
Closed

Recommended method can produce invalid sitemap #11869

ShMcK opened this issue Feb 18, 2019 · 6 comments

Comments

@ShMcK
Copy link

ShMcK commented Feb 18, 2019

Description

The Gatsby SiteMap plugin should remove any trailing slashes from the siteUrl.

const siteAddress = new Url('www.example.com')

module.exports = {
  siteMetadata: {
    siteUrl: 'www.example.com/'
  },
  plugins: [
  `gatsby-plugin-sitemap`,
  ]
}

This example will produce an invalid sitemap.

Notice the double slashes following the hostname. https://www.example.com//somePath.

<?xml version="1.0" encoding="UTF-8"?>
<url> <loc>https://www.example.com//somePath</loc></url>

Solution

The sitemap plugin just needs to remove any trailing slashes from the siteUrl before processing.

@DSchau
Copy link
Contributor

DSchau commented Feb 18, 2019

@ShMcK wouldn't it make more sense to remove the trailing slash? We could/should guard against this though with our schema validation of gatsby-config.js, though.

We should fix things early (or warn/error/etc.) rather than baking in fixes to all plugins that expect a normalized format.

Something like this would be a nice addition: https://runkit.com/dschau/5c6ae2aff8150400120f142c

@ShMcK
Copy link
Author

ShMcK commented Feb 18, 2019

I had followed a recommended pattern from the docs which produced this issue.

recipes - with cloudfront.

const siteAddress = new URL('https://www.example.com')

module.exports = {
  siteMetadata: {
    siteUrl: siteAddress.href, // which is "https://www.example.com/"
  },
  plugins: [
  `gatsby-plugin-sitemap`,
  ]
}

I suppose possible solutions are among these:

  • fix the cloudfront recipe docs
  • add a warning to the config schema validation
  • remove the trailing slash from the the siteUrl in the sitemap plugin

@DSchau
Copy link
Contributor

DSchau commented Feb 18, 2019

@ShMcK that's not really an officially maintained document - but feel free to PR with any changes as you see fit. Agreed--it's confusing!

Also note: they do document this (which would solve the issue) later on:

siteAddress.href.slice(0, -1)

Easiest course here is to just follow that approach and possibly PR any changes to the Cloudfront doc, as needed.

@ShMcK
Copy link
Author

ShMcK commented Feb 18, 2019

In that case, it may be worth replacing this line:

If you need the full address elsewhere in your config, you can access it via siteAddress.href.

to

If you need the full address elsewhere in your config, you can access it via siteAddress.href.slice(0, -1).

Which altogether doesn't seem simpler than just setting a siteAddress variable to a string.

@DSchau
Copy link
Contributor

DSchau commented Feb 18, 2019

Of course--that makes sense, yeah.

Should probably open up that issue and PR on that repo rather than this one, though.

Which altogether doesn't seem simpler than just setting a siteAddress variable to a string.

Agreed - but I think the use case for doing it that way is to programmatically grab the protocol, hostname, etc. for the s3 bucket.

I'm going to close this out - but we'd love a PR beefing up our validation here and possibly warning when a trailing slash is detected in areas like pathPrefix, siteUrl, etc.

Thanks for opening this issue!

@DSchau DSchau closed this as completed Feb 18, 2019
@DSchau
Copy link
Contributor

DSchau commented Feb 18, 2019

Opening an issue for the underlying feature request/change here too! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants