-
Notifications
You must be signed in to change notification settings - Fork 10.3k
fix(gatsby-plugin-sitemap): add meaningful error when siteUrl is missing #13123
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(gatsby-plugin-sitemap): add meaningful error when siteUrl is missing #13123
Conversation
| throw new Error(`SiteMetaData 'siteUrl' property is required`) | ||
| } | ||
|
|
||
| if (r.data.site.siteMetadata.siteUrl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could remove this if around r.data.site.siteMetadata.siteUrl for removing trailing slashes as well? Happy to do it if you would like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes sense if we do throw earlier
| }) | ||
|
|
||
| if (!r.data.site.siteMetadata.siteUrl) { | ||
| throw new Error(`SiteMetaData 'siteUrl' property is required`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about linking to https://www.gatsbyjs.org/packages/gatsby-plugin-sitemap/#how-to-use to see example configuration (to provide more context to error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked around for errors that reference links to docs, but didn't see any (swear I have before though)? If there is a standard format for this please let me know and I'll update it.
3203019 to
06fb672
Compare
wardpeet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test and added @LekoArts suggestion.
Lets merge when green
95af05a to
9f79efa
Compare
|
Ha! Was going to push a test for this this morning, thanks @wardpeet ! |
|
Published in |
…ing (gatsbyjs#13123) ## Description Throw a more descriptive error when users do not supply a `siteUrl` property when using `gatsby-plugin-sitemap` ## Related Issues Fixes gatsbyjs#12912
Description
Throw a more descriptive error when users do not supply a
siteUrlproperty when usinggatsby-plugin-sitemapRelated Issues
Fixes #12912