-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Adds extra elements to RSS items. #6707
Conversation
I found I wanted to add categories to my RSS feed, but it wasn't supported, aside from through custom data. I thought it would be better for the RSS generator to have types for the available elements than creating my own XML by hand.
🦋 Changeset detectedLatest commit: b4daee6 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 |
The Windows/Node 16 seems to have just failed due to timeout. Can someone rerun them for me? I can't restart the job myself. |
Astro always sets the <guid> element to the URL of the post. For that reason we can also set the isPermaLink attribute to true for all <guid> elements.
Apologies for keeping adding things to this PR. I actually believe it is complete now (after I updated the types and the README with the last commit). Would love a review or to know what you think? Thanks! |
@philnash, since there's no RFC to this PR, could you please expand your PR's description and explain all the new fields and why they were added? |
Hi @ematipico, thanks for stopping by this PR. I’ve updated the PR description, does that help at all? Apologies for not following the RFC process, I was just building my own site, found bits missing in the RSS generator and took it upon myself to add them. Hope the description makes more sense. Do let me know if there’s anything else I can do to improve this PR or persuade you that it’s a good idea. |
I have updated the docs in the README, upgrading the description of an I also updated There is a failing test that appears to be unrelated (it's to do with JSX). The Windows and Ubuntu versions of those tests passed, so I don't know why the Mac version is flaking on me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! @withastro/maintainers-docs , I would like your review for documentation!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this! The only thing I'm missing is explaining some differences in between the new and old version! Maybe you should add a upgrading to vX.x.x
section?
@@ -202,6 +185,112 @@ export const get = () => rss({ | |||
}); | |||
``` | |||
|
|||
## `RSSFeedItem` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how well you've restructured the docs and how well you explained this
Heads up that I'm in the middle of a review on this one! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this @philnash! I love me some RSS and I actually can't wait to set this up on my own blog! 🙌
It was also so easy to edit something so thoughtfully and carefully written. Much appreciated! You'll see a lot of "suggestions" but it's mostly just defining all the properties in a uniform way, starting with a literal description of what the thing is. (Instead of, for example, "If you want to...")
We prefer to have reference definitions start with "Just the facts, ma'am"... and then we often find that we don't need a lot of the introductory words. So, it becomes easier for the reader to scan and take in the key information more quickly. I've updated your definitions in that style, to give you an idea. But, sometimes a nuance is lost, so don't just take my changes at face value, and please DO correct anything that isn't quite right! These are just examples of the format I'd use in our docs.
I also indicated a couple of places where I felt a code example would be helpful. Again, please edit those for correctness, as I really just wanted to illustrate what I'd hope to see there.
Ping me directly with any questions, or to review your changes. And again, thank you for such an amazing contribution, and documentation that only needed some structural work to be consistent with our docs! 🥳
packages/astro-rss/README.md
Outdated
|
||
Type: `string (optional)` | ||
|
||
If the item is complete within itself, that is you are publishing the full content of the item in the feed, the `description` field may contain the full text (entity-encoded HTML is permitted). If the item is a stub, then the `description` may contain a synopsis of the item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the item is complete within itself, that is you are publishing the full content of the item in the feed, the `description` field may contain the full text (entity-encoded HTML is permitted). If the item is a stub, then the `description` may contain a synopsis of the item. | |
A synopsis of your item when you are publishing the full content of the item in the `content` field. The `description` may alternatively be the full content of the item in the feed if you are not using the `content` field (entity-coded HTML is permitted). |
I read a bit of backstory on this item, and it appears that the standard is terrible. 😅
In terms of describing this field, it is either one or the other, and if content
is used, then this should be only a summary. But, content
is newer and not yet universally supported, which is why some will use description
for full content, and that's apparently.... fine? So I think the best we can do is simply state that this is what this field is, both if you do and do not also use the content
field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, RSS is old and a mess! And feed readers have inconsistent implementations. Such fun!
packages/astro-rss/README.md
Outdated
|
||
Type: `string (optional)` | ||
|
||
If you want to supply both a short description and also the full content in an item, set the `content` field to the full, encoded text. See the [recommendations from the RSS spec for how to use and differentiate between `description` and `content`](https://www.rssboard.org/rss-profile#namespace-elements-content-encoded). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to supply both a short description and also the full content in an item, set the `content` field to the full, encoded text. See the [recommendations from the RSS spec for how to use and differentiate between `description` and `content`](https://www.rssboard.org/rss-profile#namespace-elements-content-encoded). | |
The full text content of the item suitable for presentation as HTML. If used, you should also provide a short article summary in the `description` field. | |
See the [recommendations from the RSS spec for how to use and differentiate between `description` and `content`](https://www.rssboard.org/rss-profile#namespace-elements-content-encoded). |
Something like this maybe? I prefer to start these descriptions by stating what they are, vs a "if you're doing this..."
packages/astro-rss/README.md
Outdated
|
||
Type: `string[] (optional)` | ||
|
||
If you use tags or categories to categorize your content, you can add them as the `categories` field. They will be output as multiple `<category>` elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use tags or categories to categorize your content, you can add them as the `categories` field. They will be output as multiple `<category>` elements. | |
A list of any tags or categories used to categorize your content. They will be output as multiple `<category>` elements. |
packages/astro-rss/README.md
Outdated
|
||
Type: `string (optional)` | ||
|
||
Useful for multi-author blogs, the `author` field provides the email address of the person who wrote the item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful for multi-author blogs, the `author` field provides the email address of the person who wrote the item. | |
The email address of the item author. This is useful for indicating the author of a post on multi-author blogs. |
It seems a little odd to me that author is an email address, and not just a name. So I'm just commenting on that here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but that's what the spec says. 🤷♂️
packages/astro-rss/README.md
Outdated
|
||
Type: `string (optional)` | ||
|
||
The `commentsUrl` defines a URL of a web page that contains comments on the item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `commentsUrl` defines a URL of a web page that contains comments on the item. | |
The URL of a web page that contains comments on the item. |
packages/astro-rss/README.md
Outdated
|
||
Type: `string (required)` | ||
|
||
The `url` field for the `enclosure` defines a URL where the media can be found. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `url` field for the `enclosure` defines a URL where the media can be found. | |
The URL where the media can be found. |
If you only need to use e.g. /media/filename
for media hosted on your own site, and not the full URL, then I would also add a sentence mentioning that full URLs are only required for external sources. (And, make sure the example code in enclosure
demonstrates a correct URL!)
packages/astro-rss/README.md
Outdated
|
||
The `url` field for the `enclosure` defines a URL where the media can be found. | ||
|
||
#### `length` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#### `length` | |
#### `enclosure.length` |
packages/astro-rss/README.md
Outdated
|
||
Type: `number (required)` | ||
|
||
The `length` field defines the size of the file found at the `url` in bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `length` field defines the size of the file found at the `url` in bytes. | |
The size of the file found at the `url` in bytes. |
packages/astro-rss/README.md
Outdated
|
||
The `length` field defines the size of the file found at the `url` in bytes. | ||
|
||
#### `type` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#### `type` | |
#### `enclosure.type` |
packages/astro-rss/README.md
Outdated
|
||
Type: `string (required)` | ||
|
||
The `type` field defines the MIME type for the media item found at the `url`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `type` field defines the MIME type for the media item found at the `url`. | |
The [MIME type](https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Common_types) for the media item found at the `url`. |
Thanks so much for the reviews on this. I have been traveling for speaking at a conference, so haven't had the time to go over it all. But I should be be able to go through it all this week. |
Also allows for result.commentsUrl and result.enclosure.url to be relative, using createCanonicalURL function.
Ok, updated the docs and allowed for URLs to be relative (and turned into a full URL using Let me know if this is looking good now, thanks! |
This change only adds optional fields, so there's nothing you have to do to upgrade. Do you think it needs an upgrading section? |
Thanks, @philnash this looks great! I don't think we need an upgrading section since no existing RSS feeds should break. @Yan-Thomas , would you kindly give this a comma (etc.) once-over? 😄 When Yan is happy, docs are happy! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @philnash, this is great! I added a few suggestions regarding grammar/typos for you to address 🙌
packages/astro-rss/README.md
Outdated
customData?: string; | ||
}; | ||
``` | ||
When providing a formatted RSS item list, see the [`RSSFeedItem` type reference below](#rssfeeditem). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the link name doesn't need to address the position in the page.
When providing a formatted RSS item list, see the [`RSSFeedItem` type reference below](#rssfeeditem). | |
When providing a formatted RSS item list, see the [`RSSFeedItem` type reference](#rssfeeditem) below. |
packages/astro-rss/README.md
Outdated
@@ -202,6 +185,141 @@ export const get = () => rss({ | |||
}); | |||
``` | |||
|
|||
## `RSSFeedItem` | |||
|
|||
An `RSSFeedItem` is a single item in the list of items in your feed. It represents a story, with `link`, `title` and `pubDate` fields. There are further optional fields defined below. You can also check the definitions for the fields in the [RSS spec](https://validator.w3.org/feed/docs/rss2.html#ltpubdategtSubelementOfLtitemgt). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An `RSSFeedItem` is a single item in the list of items in your feed. It represents a story, with `link`, `title` and `pubDate` fields. There are further optional fields defined below. You can also check the definitions for the fields in the [RSS spec](https://validator.w3.org/feed/docs/rss2.html#ltpubdategtSubelementOfLtitemgt). | |
An `RSSFeedItem` is a single item in the list of items in your feed. It represents a story, with `link`, `title`, and `pubDate` fields. There are further optional fields defined below. You can also check the definitions for the fields in the [RSS spec](https://validator.w3.org/feed/docs/rss2.html#ltpubdategtSubelementOfLtitemgt). |
packages/astro-rss/README.md
Outdated
|
||
Type: `object (optional)` | ||
|
||
An object that defines the `title` and `url` of the original feed for items that have been republished from another source. Both are required propeties of `source` for proper attribution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An object that defines the `title` and `url` of the original feed for items that have been republished from another source. Both are required propeties of `source` for proper attribution. | |
An object that defines the `title` and `url` of the original feed for items that have been republished from another source. Both are required properties of `source` for proper attribution. |
packages/astro-rss/README.md
Outdated
|
||
Type: `string (required)` | ||
|
||
The name of the original feed in which the item was published. (Note that this is the the feed's title, not the individual article title.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the original feed in which the item was published. (Note that this is the the feed's title, not the individual article title.) | |
The name of the original feed in which the item was published. (Note that this is the feed's title, not the individual article title.) |
Thanks @Yan-Thomas, some silly mistakes there! Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docswise approved!
Hey team, is there anything else I need to do here? Or does this just need to wait for a good time to merge? Is the build fail an issue? It seemed flaky to me, but it does put a big red cross against the PR. Can someone rerun the test to see if it will pass? Thanks |
Hey @philnash, a good part of the team is coming back and recovering from a trip last week, they will reach this PR soon and properly review the feature, so you don't need to do anything for now. And yes, the test looks flaky, I'll rerun but it's possible it might continue failing, but no need to worry, it's going to be properly analyzed by the team. |
Thanks @Yan-Thomas, didn't realise the team had been away. Hope it was a good trip. And yay! The tests passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one thing missing that I didn't see before. You have to create a changest via pnpm changeset
. I think this is a minor
change because it adds new features and it doesn't break the existing behaviour.
Changeset added @ematipico. That was nice and easy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good to me!
Changes
I found I wanted to add categories to my RSS feed, but it wasn't supported, aside from through custom data. I thought it would be better for the RSS generator to have types for the available elements than creating my own XML by hand.
I also noticed that the
<guid>
element is always set to the item's permalink, so we can set<guid isPermaLink="true">
too.Testing
Unit tests with an RSS feed with an item using all available options
Docs
/cc @withastro/maintainers-docs for feedback!
More in depth description
The RSS generation module only currently generates a subset of the available fields in the RSS spec opting to allow users to generate their own XML and supply it via the
customData
property.Since the spec is not very long, I thought it would help to expand the fields available for the
<item>
to the full set. This will allow users to add categories (tags) easily, which is what I was missing, plus links to comments, the authors email address, links to a source feed and media objects (great for podcasts).The fields added are:
This completes the spec for the items. There are additional fields that can be added to the channel as a whole, but I wanted to make small changes, especially as a first proposal. I wasn’t fully aware of the RFC process, and will look into it for future contributions.