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 <lastmod> to sitemap #9234

Closed
wants to merge 8 commits into from

Conversation

pmarschik
Copy link

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Add <lastmod> to sitemap, should fix #2604.

Test Plan

Extend existing sitemap tests to expect generated lastmod date.

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/sitemap.xml

Related issues/PRs

@facebook-github-bot
Copy link
Contributor

Hi @pmarschik!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@netlify
Copy link

netlify bot commented Aug 16, 2023

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit ddba059
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/65d602d27e9e3f0008b202bb
😎 Deploy Preview https://deploy-preview-9234--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.

@github-actions
Copy link

github-actions bot commented Aug 16, 2023

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 59 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/installation 🟠 89 🟢 98 🟢 100 🟢 100 🟠 89 Report
/docs/category/getting-started 🟠 77 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog 🟠 75 🟢 100 🟢 100 🟢 90 🟠 89 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 64 🟢 97 🟢 100 🟢 100 🟠 89 Report
/blog/tags/release 🟠 76 🟢 100 🟢 100 🟠 80 🟠 89 Report
/blog/tags 🟠 77 🟢 100 🟢 100 🟢 90 🟠 89 Report

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 19, 2023
@ardeche07
Copy link

Thanks for submitting this @pmarschik ! Have you gotten any feedback on this PR? We use Docusaurus at Unleash and were just trying to add last mod to our docs since it is becoming increasingly important for Google. Would love to see this merged.

@pmarschik
Copy link
Author

@ardeche07 I've not gotten any feedback, however I've been using the changes for our published docusaurus page for some time. When I get around it, I'll update the PR with the latest changes (its not actually 170 files changed, this PR was based on docusaurus v2)

@pmarschik pmarschik changed the base branch from main to docusaurus-v3 February 21, 2024 14:03
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.

Thanks for your contribution @pmarschik and sorry for the late review

Your PR is not really in a state I could merge, and try to implement things that are not really needed according to Google recommendations.

It should also target main and not docusaurus-v3 branch

Let me know if you want to give it another try, otherwise I'll create another PR.

More details on what we want to implement here:
#2604 (comment)

Comment on lines +97 to +104
lastmod:
routes !== undefined && options.lastmod !== false
? matchRoutes(rrRoutes, routePath).find(
({route}) => route?.exact === true,
)?.route?.lastModified
: undefined,
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not ideal to use matchRoute this way.

matchRoute can be expensive particularly for very large sites with thousands of routes

We should create the sitemap directly from the new routes attribute.

We have an util somewhere in core to do exactly that, and is what we use to create the routesPaths attribute.

export function getAllFinalRoutes(routeConfig: RouteConfig[]): RouteConfig[] {
  function getFinalRoutes(route: RouteConfig): RouteConfig[] {
    return route.routes ? route.routes.flatMap(getFinalRoutes) : [route];
  }
  return routeConfig.flatMap(getFinalRoutes);
}

Honestly I'm not a fan of the routesPaths attribute, and will likely try to remove it in favor of just routes in the future, so I'll take this opportunity to refactor the sitemap plugin to not use routesPaths and use routes instead.

* truncated to date and not include the timestamp.
* @see https://www.sitemaps.org/protocol.html#xmlTagDefinitions
*/
lastmod: boolean | 'date';
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 we really need that option? 🤔

Maybe it could be useful but afaik nobody asked for this yet

Comment on lines +70 to +71
/** Used for sitemap generation. Will be exposed in the lastmod tag. */
lastModified?: Date;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably add this to a metadata subattribute because we may assign other attributes to routes in the future and also let plugin authors assign custom ones (using an interface to allow typesafe extension through TS interface declaration merging).

These attributes should only be available on the Node side and not be part of the .docusaurus/routes.js file generated

@@ -176,6 +176,9 @@ export default function pluginContentPages(
modules: {
content: source,
},
// currently no way to get last modified date for pages
// we would have to refactor lastUpdate from docs
lastModified: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't look like necessary to add

Comment on lines +191 to +193
// currently no way to get last modified date for pages
// would have to refactor lastUpdate from docs
lastModified: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't look like necessary to add

Comment on lines +97 to +98
latestModified: _.maxBy(blogPostsOnPage, (item) => item.metadata.date)
?.metadata?.date,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

Comment on lines +237 to +238
lastModified: _.maxBy(blogPosts, (item) => item.metadata.date)
?.metadata?.date,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed?

@@ -299,6 +303,7 @@ export default async function pluginContentBlog(
items: blogPostItemsModule(items),
metadata: aliasedSource(pageMetadataPath),
},
lastModified: metadata.latestModified,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

@@ -274,6 +277,7 @@ export default async function pluginContentBlog(
sidebar: aliasedSource(sidebarProp),
content: metadata.source,
},
lastModified: metadata.date,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have metadata.lastUpdatedAt on blog posts too now

Comment on lines +502 to +503
/** Latest modified date of posts on the current page. */
readonly latestModified?: Date;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed

@pmarschik
Copy link
Author

Thanks for your contribution @pmarschik and sorry for the late review

Thank you for reviewing the MR @slorber.

Your PR is not really in a state I could merge, and try to implement things that are not really needed according to Google recommendations.

It should also target main and not docusaurus-v3 branch

I'm aware, this is just what I use to pnpm patch our build and have a working lastmod until the feature is available upstream.

Let me know if you want to give it another try, otherwise I'll create another PR.

I'm currently occupied with other projects and I don't know when I next have time to circle back to this.
So I think it would be best if you create another PR.

@slorber
Copy link
Collaborator

slorber commented Mar 15, 2024

Thanks

I'll close this one in favor of a new one, track #9954

@slorber slorber closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add lastmod to sitemap
5 participants