Skip to content

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

Merged
ematipico merged 10 commits intowithastro:mainfrom
florian-lefebvre:fix/optional-rss-title
Jan 6, 2024
Merged

fix(rss): make title optional if description is provided#9610
ematipico merged 10 commits intowithastro:mainfrom
florian-lefebvre:fix/optional-rss-title

Conversation

@florian-lefebvre
Copy link
Copy Markdown
Member

@florian-lefebvre florian-lefebvre commented Jan 4, 2024 •
edited
Loading

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jan 4, 2024 •
edited
Loading

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

Princesseuh commented Jan 4, 2024

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

Princesseuh
Princesseuh reviewed Jan 4, 2024
View reviewed changes
Princesseuh
Princesseuh reviewed Jan 4, 2024
View reviewed changes
florian-lefebvre and others added 2 commits January 4, 2024 22:11
@florian-lefebvre @Princesseuh
Update packages/astro-rss/test/pagesGlobToRssItems.test.js
dc87abf 
Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
@florian-lefebvre @Princesseuh
Update packages/astro-rss/test/pagesGlobToRssItems.test.js
72e1118 
Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
MoustaphaDev
MoustaphaDev approved these changes Jan 4, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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:

astro/packages/astro-rss/src/index.ts

Lines 117 to 123 in 2814984

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)

astro/packages/astro-rss/src/index.ts

Lines 212 to 221 in 2814984

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' },
};

florian-lefebvre
florian-lefebvre commented Jan 5, 2024
View reviewed changes
packages/astro-rss/src/index.ts
? result.link
: createCanonicalURL(result.link, rssOptions.trailingSlash, site).href;
item.link = itemLink;
item.guid = { '#text': itemLink, '@_isPermaLink': 'true' };
Copy link
Copy Markdown
Member Author

@florian-lefebvre florian-lefebvre Jan 5, 2024

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

@florian-lefebvre florian-lefebvre requested a review from bluwy January 5, 2024 09:17
@florian-lefebvre florian-lefebvre mentioned this pull request Jan 5, 2024
Merged
bluwy
bluwy approved these changes Jan 5, 2024
View reviewed changes
Copy link
Copy Markdown
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.

ematipico
ematipico approved these changes Jan 5, 2024
View reviewed changes
@florian-lefebvre @ematipico
Update .changeset/shy-spoons-sort.md
1666e75 
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
@ematipico ematipico merged commit 24663c9 into withastro:main Jan 6, 2024
@astrobot-houston astrobot-houston mentioned this pull request Jan 6, 2024
Merged
@florian-lefebvre florian-lefebvre deleted the fix/optional-rss-title branch January 6, 2024 08:21
robmen
robmen reviewed Jan 8, 2024
View reviewed changes
packages/astro-rss/test/pagesGlobToRssItems.test.js
return chai.expect(pagesGlobToRssItems(globResult)).to.not.be.rejected;
});

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

@robmen robmen Jan 8, 2024

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
Copy Markdown
Member Author

@florian-lefebvre florian-lefebvre Jan 8, 2024

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
Copy Markdown
Contributor

@robmen robmen Jan 8, 2024

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
Copy Markdown
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.

@robmen robmen mentioned this pull request Jan 8, 2024
Merged
bluwy pushed a commit to withastro/docs that referenced this pull request Jan 11, 2024
@florian-lefebvre @sarah11918
Update docs for withastro/astro#9610 (#6154)
09a2136 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@wkillerud wkillerud mentioned this pull request Jan 19, 2024
Closed
1 task
ematipico pushed a commit to withastro/docs that referenced this pull request Jan 26, 2024
@florian-lefebvre @sarah11918 @ematipico
Update docs for withastro/astro#9610 (#6154)
aa693a0 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
This was referenced Oct 6, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ematipico ematipico ematipico approved these changes

@MoustaphaDev MoustaphaDev MoustaphaDev approved these changes

@Princesseuh Princesseuh Awaiting requested review from Princesseuh

+2 more reviewers

@robmen robmen robmen left review comments

@bluwy bluwy bluwy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr: docs A PR that includes documentation for review

Projects

None yet

Milestone

No milestone

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

@florian-lefebvre @Princesseuh @bluwy @robmen @ematipico @MoustaphaDev