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

Include dynamically rendered pages in sitemap #812

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

OzzieOrca
Copy link

@OzzieOrca OzzieOrca commented May 15, 2024

Add manifest.routes.staticRoutes to allKeys in createUrlSet.

Just taking a very naive stab at this, not sure this is even the right part of the manifest to pull these dynamically rendered paths from but I found my missing pages inside of it. staticRoutes seems like a weird name for the dynamic pages but that's where I found them. This seems to pick up some of the pages that went missing as soon as I called headers() or cookies() from import { headers, cookies } from 'next/headers'; in my page or layout. I've been running this patch in production since May.

Opting into dynamic rendering seems to put the page in a different place in the build output manifest:

headers() is a Dynamic Function whose returned values cannot be known ahead of time. Using it in a layout or page will opt a route into dynamic rendering at request time.

https://nextjs.org/docs/app/api-reference/functions/headers

Does this approach seem like a good direction? Any feedback? Just trying to start a conversation and hopefully solve #692 and #743.

Looks like there was an earlier attempt at #759.

I'm testing with Next v14.2.3 and patched next-sitemap v4.2.3 locally to test.

Add staticRoutes to allKeys in createUrlSet
Copy link

vercel bot commented May 15, 2024

@OzzieOrca is attempting to deploy a commit to the Vishnu Sankar's projects Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

Closing this PR due to inactivity.

@OzzieOrca
Copy link
Author

@iamvishnusankar Any direction/suggestions here? This issue has plagued me on and off for over a year. Seems like there are several issues out there where some pages are ignored when generating sitemaps.

@nitinTJ
Copy link

nitinTJ commented Jul 16, 2024

I would suggest you to create your own sitemap generation script.

@OzzieOrca
Copy link
Author

Hmm, that's not the answer I was hoping for. Per the description of this project,

Sitemap generator for next.js. Generate sitemap(s) and robots.txt for all static/pre-rendered/dynamic/server-side pages.

it would seem like adding "dynamic"ally rendered pages to the generated sitemap would be in the scope of this project.

I think it works as desired with the pages router. But when using the new app router and calling headers() or cookies() this library excludes the dynamic pages that used to be included. Which makes this issue feel more like a bug and not a feature request.

If I wanted to maintain a manually written sitemap or a custom sitemap generation script script, I think I could do it with just the Next.js generating a sitemap using code feature. But my goal was not to have to edit the sitemap every time I add new pages to the site or have to maintain and update a custom script every time we add new types of page collections. And we integrated this library for its automatic sitemap generation features which Next.js doesn't provide.

Right now I'm running a patched version of this library with the changes in the PR and it's working well. I was just hoping to get responses and feedback and see if we can improve this experience for everyone and fix this for users who have reported related issues.

@rklos
Copy link

rklos commented Aug 23, 2024

+1 for this feature

Copy link

Closing this PR due to inactivity.

@OzzieOrca
Copy link
Author

I think this should still be open, unless it's been solved elsewhere I haven't seen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants