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

feat: add createSitemapItems hook #10083

Merged
merged 8 commits into from Apr 30, 2024

Conversation

johnnyreilly
Copy link
Contributor

@johnnyreilly johnnyreilly commented Apr 28, 2024

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:

  1. To add lastmod to sitemap entries (something that is no longer necessary as of 3.2)
  2. To trim pagination, tags pages and programmatically determined canonicals from the sitemap - this is still necessary

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

image

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.xml
image

Test 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

Copy link

netlify bot commented Apr 28, 2024

[V2]

Name Link
🔨 Latest commit 61ae607
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/6631323dbce6020008440554
😎 Deploy Preview https://deploy-preview-10083--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

github-actions bot commented Apr 28, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 60 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 58 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 70 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 62 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 70 🟢 100 🟢 100 🟠 80 🟠 88 Report
/blog/tags 🟠 74 🟢 100 🟢 100 🟢 90 🟠 88 Report

Copy link
Collaborator

@slorber slorber left a 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};
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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;
Copy link
Collaborator

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?

@johnnyreilly
Copy link
Contributor Author

Just pushed a change that omits head and options from params - is this what you had in mind?

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Apr 30, 2024
Copy link
Collaborator

@slorber slorber left a 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!

@slorber slorber merged commit 7057ba4 into facebook:main Apr 30, 2024
31 checks passed
@johnnyreilly johnnyreilly deleted the createsitemapitems branch April 30, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: createSitemapItems hook - a sitemap equivalent to createFeedItems
3 participants