-
-
Notifications
You must be signed in to change notification settings - Fork 8.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
feat(blog): add options.createFeedItems to filter/limit/transform feed items #8378
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
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.
See comment
Reworked this based on feedback to replace the createFeedItems?: (options: {
blogPosts: BlogPost[];
siteConfig: DocusaurusConfig;
outDir: string;
defaultCreateFeedItems: (params: {
blogPosts: BlogPost[];
siteConfig: DocusaurusConfig;
outDir: string;
}) => Promise<BlogFeedItem[]>;
}) => Promise<BlogFeedItem[]>; Usage looks like this: createFeedItems: async (options) => {
const { blogPosts, defaultCreateFeedItems, ...others } = options;
const blogPostsFiltered = blogPosts.filter((item, index) => index < 2);
return defaultCreateFeedItems({ blogPosts: blogPostsFiltered, ...others });
} The above implements the a filter; merely one of many things that could be done. |
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, looks almost good 👍
packages/docusaurus-plugin-content-blog/src/plugin-content-blog.d.ts
Outdated
Show resolved
Hide resolved
@@ -54,11 +55,35 @@ async function generateBlogFeed({ | |||
copyright: feedOptions.copyright, | |||
}); | |||
|
|||
const feedItems = await (feedOptions.createFeedItems |
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.
maybe simplify with:
const createFeedItems = feedOptions.createFeedItems ?? defaultCreateFeedItems
?
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.
Would love to but the signatures are subtly different. Did ponder other simplifications but didn't come up with anything that seemed worthwhile. Happy to implement any other ideas?
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.
Would it be a big problem if the 2 functions had the exact same signature? It shouldn't harm anyone if defaultCreateFeedItems would "receive itself" as a param, although it wouldn't be used in practice.
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.
gave it a try and apparently TS is fine with that thanks to duck typing 👍 updated the PR
LGTM Need a bit of docs in https://deploy-preview-8378--docusaurus-2.netlify.app/docs/api/plugins/@docusaurus/plugin-content-blog/#configuration Also remove current test before merge ;) |
Awesome! I'll add some docs shortly |
Nice - |
…d items (#8378) Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
Glad you submitted that PR @johnnyreilly 😄 That will allow me to stay under Vercel fair use free plan for a little bit more. My 4MO feed is exploding the plan otherwise. Wonder if we shouldn't limit by default. Considering we output blog content in the feed and it's not rare to see blogs with 100+ blog posts. Wonder what would be a good default limit (20?) and what would be the API to change it or disable it in case you really want all blog posts in your feed. |
Haha - glad it's of use to you! Yeah I did wonder about this. I think the other thing to factor in is that it's the size of the feed that's the issue for consumers generally, and that's not just a factor of the number of blog entries. But also the length of the underlying posts. It'd be nice if it could limit by size of feed, including the max number of entries that keeps the feed under a reasonable size. Though maybe this is too complicated to expose to users. The idea of a default of a max number of entries of 20 seems reasonable. Again, being able to opt out is probably wise. |
That seems hard to achieve with our current system and a bit overkill. We write the XML at once from in-memory data and don't stream to it with the ability to stop at a given time. Limiting the number of entries is much simpler and probably good enough. I guess we can have a Let me know if you want to submit a PR |
Yeah I'd be happy to submit a PR; I'm on holiday right now but maybe sometime in August. Feel free to ping me if I forget |
👍 Here's an issue: #9179 For some reason I can't assign it to you |
That's funny - I can't assign it to myself either. Maybe I have to be added as a member to the Docusaurus project to be assigned something? Not sure |
I can assign the issue to many john... persons but not you, you don't appear in the autocomplete 😅 maybe it's a profile setting? 🤔 |
Maybe - mysterious! |
I think you can only assign to people who have participated and people affiliated with the organization? |
Pre-flight checklist
THIS IS AN EXAMPLE IMPLEMENTATION TO ILLUSTRATE #8376
Motivation
My blog turns out to be quite large. It was recently brought to my attention that the RSS feed wasn't working well for readers as a consequence.
At present, Docusaurus includes all blog entries in the Atom / RSS feeds: https://docusaurus.io/docs/blog#feed
The proposal is to add another option to support filtering the responses in the feed. This would allow people to reduce the size of their blogs when they become large.
If this existed, then this would not be necessary: johnnyreilly/blog.johnnyreilly.com#352
Test Plan
See automated tests and observe the
atom.xml
below - 2 entries only due to filtering:or go here: https://deploy-preview-8378--docusaurus-2.netlify.app/blog/rss.xml
Test links
Deploy preview: https://deploy-preview-8378--docusaurus-2.netlify.app/blog/rss.xml
Docs:
Related issues/PRs
fix #8376