-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
feat: add createSitemapItems hook #10083
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
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.
Overall that looks good, will review in depth later
type CreateSitemapItemsFn = (params: { | ||
siteConfig: DocusaurusConfig; | ||
routes: RouteConfig[]; | ||
head: {[location: string]: HelmetServerState}; |
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 know postBuild
already receives this, but IMHO this was a mistake and we shouldn't expose this "messy" data structure as part of our public API.
Unless it's really needed I'd prefer to remove it for now, or provide it with a clear Docusaurus public API.
Also React 19 has metadata apis now, so not sure it's a good time to provide such API. We can introduce it later once things get clearer.
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.
The reason the API is this shape is because it is modelled on the createFeedItems
API which allows the caller to call the underlying default implementation and provides the necessary inputs to do so. We could have a different style of API here which doesn't work that way. Instead, perhaps, we could expose a "default" implementation which wraps the underlying dependencies of the actual implementation and just exposes a parameterless function that could be called. That would likely feel cleaner.
What do you think? The downside of this is inconsistency. The createFeedItems
and createSitemapItems
APIs will be similarly named but slightly different to use from one another. Not necessarily a big issue - but worth considering
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'm not sure we talk about the same thing. What annoys me most is this {[location: string]: HelmetServerState};
And annoying code like this:
// Maybe we want to add a routeConfig.metadata.noIndex instead?
// But using Helmet is more reliable for third-party plugins...
function isNoIndexMetaRoute({
head,
route,
}: {
head: {[location: string]: HelmetServerState};
route: string;
}) {
const isNoIndexMetaTag = ({
name,
content,
}: {
name?: string;
content?: string;
}): boolean => {
if (!name || !content) {
return false;
}
return (
// meta name is not case-sensitive
name.toLowerCase() === 'robots' &&
// Robots directives are not case-sensitive
content.toLowerCase().includes('noindex')
);
};
// https://github.com/staylor/react-helmet-async/pull/167
const meta = head[route]?.meta.toComponent() as unknown as
| ReactElement<{name?: string; content?: string}>[]
| undefined;
return meta?.some((tag) =>
isNoIndexMetaTag({name: tag.props.name, content: tag.props.content}),
);
}
I'd prefer if we didn't expose HelmetServerState
to the user. I don't think createFeedItems
exposes it so far.
siteConfig: DocusaurusConfig; | ||
routes: RouteConfig[]; | ||
head: {[location: string]: HelmetServerState}; | ||
options: PluginOptions; |
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.
wonder if this one is really useful
I know createFeedItem takes the config (which IMHO is already not super useful), but it doesn't take the option so maybe we shouldn't add this here?
Just pushed a change that omits |
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.
LGTM thanks 👍
Did some minor code/docs/type refactors but overall it works!
Pre-flight checklist
Motivation
I presently mutate my sitemap manually on each build as a post processing step. I've written about it here and I have historically done this for two reasons:
lastmod
to sitemap entries (something that is no longer necessary as of 3.2)If there was a hook that allowed control of the sitemap, I would no longer need to do 2 as a separate post processing step
Test Plan
Automated tests, plus look at the generated sitemap for the preview site: https://deploy-preview-10083--docusaurus-2.netlify.app/sitemap.xml
The screenshot below was taken from the current live Docusaurus sitemap: https://docusaurus.io/sitemap.xml
Please note the URLs above which feature
page
. Now look at the sitemap generated using this PR which filters those sitemap items out out using the new hook: https://deploy-preview-10083--docusaurus-2.netlify.app/sitemap.xmlTest links
Deploy preview sitemap: https://deploy-preview-10083--docusaurus-2.netlify.app/sitemap.xml
Updated docs: https://deploy-preview-10083--docusaurus-2.netlify.app/docs/api/plugins/@docusaurus/plugin-sitemap
Related issues/PRs
#10081