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

[RSS] Get ready for content collections #5851

Merged
merged 37 commits into from
Jan 19, 2023
Merged

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Jan 13, 2023

Changes

  • Expose rssSchema to type-check content collections for RSS feeds
  • Deprecate internal import.meta.glob handling and move to a pagesGlobToRssItems() helper. This has a few benefits:
    • Simplifies our items array to accept a single type. Better for JS docs and docs.astro.build.
    • Makes the "only glob src/pages/ entries" requirement more explicit with the function name.
    • Makes handling globs and handling content collections feel equally supported. When import.meta.glob handling was implicit, this felt like the more encouraged solution.
  • Refactor internals to use Zod all the way down. This makes error handling on bad config much clearer!

README changes

  • Change glob example to a content collections example
  • Add rssSchema and pagesGlobToRssItems() reference
  • Chore: change import.meta.env.SITE recommendation to new context.site

Testing

  • Update testing for new APIs
  • Migrate glob testing to separate pagesGlobToRssItems() tests

Docs

@bholmesdev bholmesdev requested a review from a team as a code owner January 13, 2023 15:21
@changeset-bot
Copy link

changeset-bot bot commented Jan 13, 2023

🦋 Changeset detected

Latest commit: a3cd5e9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: example Related to an example package (scope) label Jan 13, 2023
@bholmesdev
Copy link
Contributor Author

Updating to keep import.meta.glob() handling around as a deprecated API, and dropping to a non-breaking minor!

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Seems like a great refactor! Had a few comments around messaging and maybe found a typo?

.changeset/fluffy-cups-travel.md Outdated Show resolved Hide resolved
packages/astro-rss/src/index.ts Show resolved Hide resolved
@bholmesdev bholmesdev force-pushed the feat/rss-content-collections branch from 1192595 to c739709 Compare January 18, 2023 21:11
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Quick Docs README review:

I've reviewed the accompanying PR to docs, which I know contains some of the same examples and sections. I'll let you look over those comments, and decide which ones are applicable here. It's OK if this README is more technical than the docs RSS page, which should be more oriented to practical usage.

But, I want to give you a chance to look over that review and
a) see if there's any suggestion there that you'd choose to carry into here.
b) decide whether there's anything you're happy to leave exclusively to docs, and cut from the README. (We favour as little duplication as possible, and keeping the READMES to a minimum, where it makes sense. This document won't often have to stand on its own since readers are far more likely to encounter the docs page, and search only works in docs proper, etc.)

Then, I'll take another last look over this, but I don't see anything that's going to hold this up!

@bholmesdev
Copy link
Contributor Author

@sarah11918 Made some tweaks based on docs feedback! Seems good-to-go from my end 👍

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just re-add that missing "we" and good to go from Docs, @bholmesdev!

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@bholmesdev bholmesdev merged commit 81dce94 into main Jan 19, 2023
@bholmesdev bholmesdev deleted the feat/rss-content-collections branch January 19, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: example Related to an example package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants