gammastream / scully-plugins

A collection of plugins written for Scully
31 stars 9 forks source link

Sitemap pluging not handling the urlPrefix correctly #16

Closed BlindDespair closed 3 years ago

BlindDespair commented 3 years ago

Hello. Thank you for the great plugin, I've been using it for over half a year and been quite happy, but now I am working on the multilanguage feature and I tried setting:

setPluginConfig(sitemapPlugin, {
  urlPrefix: `https://example.com${BASE_HREF.replace(/\/$/, '')}`,
  sitemapFilename: 'sitemap.xml',
  changeFreq: 'monthly',
  priority: ['1.0', '0.9', '0.8', '0.7', '0.6', '0.5', '0.4', '0.3', '0.2', '0.1', '0.0'],
});

And the result is this:

<?xml version="1.0"?>
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
  <url>
    <loc>https://example.com/uk</loc>
    <changefreq>monthly</changefreq>
    <lastmod>2021-01-17T18:33:40.858Z</lastmod>
    <priority>1.0</priority>
  </url>
  <url>
    <loc>https://example.com/route1</loc>
    <changefreq>monthly</changefreq>
    <lastmod>2021-01-17T18:33:40.858Z</lastmod>
    <priority>0.9</priority>
  </url>
</urlset>

But what I expected is:

<?xml version="1.0"?>
<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
  <url>
    <loc>https://example.com/uk</loc>
    <changefreq>monthly</changefreq>
    <lastmod>2021-01-17T18:33:40.858Z</lastmod>
    <priority>1.0</priority>
  </url>
  <url>
    <loc>https://example.com/uk/route1</loc>
    <changefreq>monthly</changefreq>
    <lastmod>2021-01-17T18:33:40.858Z</lastmod>
    <priority>0.9</priority>
  </url>
</urlset>

So as you can see for non-root routes the /uk part got stripped and that is not the right result for me.

msacket commented 3 years ago

Thanks for the report. I have not used the multi-language feature yet. I'll spin up a sample project and see what I can figure out.

BlindDespair commented 3 years ago

@msacket I know exactly why this issue is happening. In the line 136 you use url.resolve() and that removes the existing part of the URL after the hostname. Here is a screenshot from their npm description: image I would recommend to replace it with something more custom that can also handle URLs with baseHref.

BlindDespair commented 3 years ago

@msacket Have you had time to check this yet? Would you like me to submit a PR? I suppose the url.resolve() was needed there to prevent // from happening if someone used https://example.com/ in their config and that would result in https://example.com//route1, right? If that's the case I could suggest using a helper function something like:

export function mergePaths(base: string, path: string): string {
  if (base.endsWith('/') && path.startsWith('/')) {
    return `${base}${path.substr(1)}`;
  }
  if (!base.endsWith('/') && !path.startsWith('/')) {
    return `${base}/${path}`;
  }
  return `${base}${path}`;
}

It would take care of slashes and not remove anything important. What do you think?

msacket commented 3 years ago

Hi @BlindDespair. I'm sorry for the delay. I've had a brief look... then I got sidetracked with i18n configuration. I've not setup a localized project so I wasn't exactly sure how to test the fix. Your fix looks good. If you want to submit a PR, I'd appreciate that. I'll merge and push an update to npm later today.

BlindDespair commented 3 years ago

@msacket Thank you for quick response, I opened a PR, you can review it whenever you have time. :)

msacket commented 3 years ago

You're welcome. Thanks for the PR. Looks good to me. I'll merge it, test it locally and then push the npm update.

msacket commented 3 years ago

@BlindDespair The npm is updated with the changes: @gammastream/scully-plugin-sitemap@1.0.7