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

fix(rss): make title optional if description is provided #9610

Merged
merged 10 commits into from
Jan 6, 2024

Conversation

florian-lefebvre
Copy link
Member

@florian-lefebvre florian-lefebvre commented Jan 4, 2024

Copy link

changeset-bot bot commented Jan 4, 2024

🦋 Changeset detected

Latest commit: 1666e75

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 docs pr A PR that includes documentation for review label Jan 4, 2024
@Princesseuh
Copy link
Member

Seems like there's a test that see if we throw an error on title, should probably be removed!

florian-lefebvre and others added 2 commits January 4, 2024 22:11
Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
Copy link
Member

@MoustaphaDev MoustaphaDev left a comment

Choose a reason for hiding this comment

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

LGTM!

@bluwy
Copy link
Member

bluwy commented Jan 5, 2024

Following #9577 (comment), could we also make pubDate and link optional? We'd also need to update this error message:

if (path === 'items' && code === 'invalid_union') {
return [
message,
`The \`items\` property requires properly typed \`title\`, \`pubDate\`, and \`link\` keys.`,
`Check your collection's schema, and visit https://docs.astro.build/en/guides/rss/#generating-items for more info.`,
].join('\n');
}

And possibly handling for undefined title and link here. (Maybe title is already fine with undefined, but maybe a test to make sure would be nice)

root.rss.channel.item = items.map((result) => {
// If the item's link is already a valid URL, don't mess with it.
const itemLink = isValidURL(result.link)
? result.link
: createCanonicalURL(result.link, rssOptions.trailingSlash, site).href;
const item: any = {
title: result.title,
link: itemLink,
guid: { '#text': itemLink, '@_isPermaLink': 'true' },
};

? result.link
: createCanonicalURL(result.link, rssOptions.trailingSlash, site).href;
item.link = itemLink;
item.guid = { '#text': itemLink, '@_isPermaLink': 'true' };
Copy link
Member Author

Choose a reason for hiding this comment

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

Since it depends on link, I add it here. But I don't know if this property is supposed to be something else when there is no link

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for fixing the other fields as well.

.changeset/shy-spoons-sort.md Outdated Show resolved Hide resolved
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
@ematipico ematipico merged commit 24663c9 into withastro:main Jan 6, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 6, 2024
@florian-lefebvre florian-lefebvre deleted the fix/optional-rss-title branch January 6, 2024 08:21
return chai.expect(pagesGlobToRssItems(globResult)).to.not.be.rejected;
});

it('should fail on missing "description" key if "title" is present', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: I think should fail should be should not fail to correctly match the behavior of the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes you're right! Feel free to submit a PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Done. See #9646

@robmen
Copy link
Contributor

robmen commented Jan 8, 2024

@florian-lefebvre I was looking closer at this fix to learn from it and I think there is a typo in the description of the test. See comment review above. Minor nit.

bluwy pushed a commit to withastro/docs that referenced this pull request Jan 11, 2024
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
ematipico pushed a commit to withastro/docs that referenced this pull request Jan 26, 2024
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The RSS spec does not require an item title, but astrojs/rss does
6 participants