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

Dynamic Sitemap Copy Plugin (#1232) #1260

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jstockdi
Copy link
Contributor

Continuing work on #1245

Related Issue

#1232

Summary of Changes

Adds a package with unit test example

[X] Built plugin
[X] Created test
[] Added jsdoc

Give some comments and I will clean it up...

Copy link
Member

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Hey @jstockdi , thanks for plugging away at this.

Not sure if you had a chance to review my thoughts from the last PR? This one seems to be going in a couple different directions and so just want to make sure we're on the same page.

For example a couple observations that are unclear to me at an immediate glance:

  • adapters are more intended for preparing "standard" Greenwood output to other hosting targets, like Netlify and Vercel (mainly for SSR pages and API routes)
  • I think if a static sitemap is supported out of the box, why not the dynamic one too? (as opposed to through a plugin). Or maybe all sitemap behaviors should be a plugin?

The main thing I see that I think we need to consider is giving the user a hook to generate their own sitemap, as opposed to us trying to it all for them.

Although not in Greenwood yet, one future facing feature we hope to get around to soon is dynamic routing, e.g. src/pages/blog/[slug].js and so I wonder how Greenwood (or Next for that matter) handle such an open ended sitemap (if not using getStaticPaths)

So would be good to consider how this might interplay with that. 🤔


I think I might put this one draft though for now and then we can discuss and add more details after our next meeting sync.

@thescientist13 thescientist13 linked an issue Jul 23, 2024 that may be closed by this pull request
@thescientist13 thescientist13 marked this pull request as draft July 23, 2024 18:26
@thescientist13 thescientist13 removed their assignment Jul 23, 2024
@jstockdi
Copy link
Contributor Author

Definitely can move the adapter into the out of the box... Following the pattern that existed as I didn't see any adapters in the box. I figured you can show me how/where to move it.

Why adapters? See the quote below...

"For build time, we maybe add something to the build step like we do for adapters, where if we have a sitemap (perhaps the graph can track this on init?) we import the that function from the user's file and write out the file for them?"

From what I see, the hook is there... and the above quote was largely followed. For the user, if is a sitemap.xml.js is created with the export, it will can generate their own sitemap. So it seems like we've given the user a hook to generate their own sitemap, as opposed to us trying to it all for them. I would prefer to see the function passed into the plugin config, but started here to evoke the discussion.

Looking forward to discussing with you.

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.

Sitemap Generation
2 participants