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(gatsby-plugin-sitemap): add meaningful error when siteUrl is missing #13123

Merged
merged 4 commits into from
Apr 5, 2019

Conversation

sbardian
Copy link
Contributor

@sbardian sbardian commented Apr 4, 2019

Description

Throw a more descriptive error when users do not supply a siteUrl property when using gatsby-plugin-sitemap

Related Issues

Fixes #12912

@sbardian sbardian changed the title Topics/sitemap site url missing Throw error if siteUrl missing when using gatsby-plugin-sitemap Apr 4, 2019
if (!r.data.site.siteMetadata.siteUrl) {
throw new Error(`SiteMetaData 'siteUrl' property is required`)
}

if (r.data.site.siteMetadata.siteUrl) {
Copy link
Contributor Author

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?

Copy link
Contributor

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

@@ -29,6 +29,10 @@ export const runQuery = (handler, query, excludes, pathPrefix) =>
return page
})

if (!r.data.site.siteMetadata.siteUrl) {
throw new Error(`SiteMetaData 'siteUrl' property is required`)
Copy link
Contributor

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)

Copy link
Contributor Author

@sbardian sbardian Apr 4, 2019

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.

@LekoArts LekoArts added the status: awaiting author response Additional information has been requested from the author label Apr 5, 2019
Copy link
Contributor

@wardpeet wardpeet left a 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

@wardpeet wardpeet changed the title Throw error if siteUrl missing when using gatsby-plugin-sitemap fix(gatsby-plugin-sitemap): add meaningful error when siteUrl is missing Apr 5, 2019
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 5, 2019
@wardpeet wardpeet force-pushed the topics/sitemap-siteUrl-missing branch from 95af05a to 9f79efa Compare April 5, 2019 12:05
@wardpeet wardpeet removed the status: awaiting author response Additional information has been requested from the author label Apr 5, 2019
@wardpeet wardpeet merged commit 65693d3 into gatsbyjs:master Apr 5, 2019
@sbardian
Copy link
Contributor Author

sbardian commented Apr 5, 2019

Ha! Was going to push a test for this this morning, thanks @wardpeet !

@sidharthachatterjee
Copy link
Contributor

Published in gatsby-plugin-sitemap@2.0.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants