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

(gatsby-plugin-sitemap) omit assetPrefix from page URLs #31454

Closed
wants to merge 3 commits into from
Closed

(gatsby-plugin-sitemap) omit assetPrefix from page URLs #31454

wants to merge 3 commits into from

Conversation

nonAlgebraic
Copy link
Contributor

@nonAlgebraic nonAlgebraic commented May 17, 2021

Description

The sitemap plugin uses an internal function prefixPath to append a site's base URL, as well as its path prefix if one is set, to the URL of each serialized page in the sitemap. The path prefix is retrieved from the pathPrefix string available via the arguments to the onPostBuild hook. This string is a concatenation of the assetPrefix and pathPrefix configuration options. Currently, the sitemap plugin naively uses this string in generating sitemap page URLs, when in fact only the pathPrefix string should be part of a page URL in the sitemap and the assetPrefix should not.

This PR modifies the prefixPath function, such that it avoids also including an asset prefix if one is set in gatsby-config. This is done by retrieving the assetPrefix configuration option and passing it to prefixPath so that it can then be omitted from the prefix, if it is indeed present.

Notes

In this PR the assetPrefix value is retrieved via the Redux store passed in the onPostBuild hook's arguments. This argument is marked as "use only if you have to" in the Gatsby source code . I was not able to find any public/"safe" API for accessing the value of the assetPrefix config option inside onPostBuild.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 17, 2021
@TylerBarnes TylerBarnes added status: needs core review Currently awaiting review from Core team member and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 28, 2021
@antdking
Copy link

It's undocumented, but basePath is exposed to Node Plugins, which is equivalent to config.pathPrefix. I think the pages in the sitemap should probably use that instead (added in v2.20, so should be available).

This is how gatsby-plugin-manifest resolved the same bug: #20142

@nonAlgebraic
Copy link
Contributor Author

@antdking this is great feedback! I'm going to close this PR and open a new one with your much cleaner solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs core review Currently awaiting review from Core team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants