-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
feat: support content on RSS Feeds #5144
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
Conversation
* @see {https://github.com/mdx-js/mdx/discussions/1985} | ||
*/ | ||
export const renderMarkdown = async body => { | ||
const { default: mdx } = await evaluate(body, { |
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.
Hmm, afaik we already should have a rendered version of this at the time, no?
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.
Where exists renderred version?
post
object just have file path…
{
post: {
title: 'Node v19.4.0 (Current)',
author: 'Rafael Gonzaga',
date: 2023-01-06T13:16:26.671Z,
category: 'release',
slug: '/blog/release/v19.4.0',
file: 'v19.4.0.md'
}
}
{
post: {
title: 'Node v18.13.0 (LTS)',
author: 'Danielle Adams',
date: 2023-01-06T01:01:33.599Z,
category: 'release',
slug: '/blog/release/v18.13.0',
file: 'v18.13.0.md'
}
}
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 think this is just what we're passing as a prop, you should check the parent functions, as I think I manually build this "post" object. The actual source of the files are somewhere in the parent-callers.
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.
Good stuff! I think you're in the right direction, but I've left some comments here about what I believe should be changed 🙇
@@ -1,8 +1,13 @@ | |||
// @TODO: This is a temporary hack until we migrate to the `nodejs/nodejs.dev` codebase | |||
import { writeFile } from 'fs/promises'; | |||
import { writeFile, readFile} from 'fs/promises'; |
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 guess you need to run npm run format
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.
Why not add a prettier check GitHub ci action rather than catch format inconsistency?
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.
We do have one. I'm pretty sure the CI failed for that.
import { evaluate } from '@mdx-js/mdx' | ||
import { createElement } from 'react' | ||
import { renderToString } from 'react-dom/server' | ||
import remarkGfm from 'remark-gfm' |
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.
Do we need remark and remark-gfm here? I thought mdx would already include remark built-ins.
remarkPlugins: [remarkGfm, remarkFrontmatter], | ||
}) | ||
return renderToString(createElement(mdx)) | ||
} |
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.
Add a blank line between this function and the next one?
@@ -43,16 +67,20 @@ export const generateWebsiteFeeds = cachedBlogData => | |||
? `/en/blog/${metadata.blogCategory}` | |||
: '/en/blog'; | |||
|
|||
const mapBlogPostToFeed = post => | |||
const mapBlogPostToFeed = async post => { | |||
const markdownContent = await readFile(join(blogPath, post.category, post.file), 'utf8'); |
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.
We're over-engineering things here. The whole already rendered blog post should be provided directly from here https://github.com/nodejs/nodejs.org/blob/main/next.data.mjs#L16.
Nextra already gives us the whole tree containing the full grey-matter, frontmatter and markdown content + parsed markdown content, afaik.
If it doesn't provide the parsed markdown into HTML, then at least we don't need to re-read the file with readFile
here 👀
Also we don't need to use async
and await
here... We can just use plain promises. renderMarkdown.then
/** | ||
* Render markdown to html via mdx | ||
* @param {string} body | ||
* @returns {Promise<string>} | ||
* @see {https://github.com/mdx-js/mdx/discussions/1985} | ||
*/ | ||
export const renderMarkdown = async body => { | ||
const { default: mdx } = await evaluate(body, { | ||
...runtime, | ||
format: "md", | ||
development: false, | ||
// format: 'md' — treat file as plain vanilla markdown | ||
// Need to add the following remark plugins to support GFM and frontmatter | ||
// https://github.com/remarkjs/remark-gfm | ||
// https://github.com/remarkjs/remark-frontmatter | ||
remarkPlugins: [remarkGfm, remarkFrontmatter], | ||
}) | ||
return renderToString(createElement(mdx)) | ||
} |
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 referred to mdx-js/mdx#1985, but there may be a better way.
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.
To covert all the ".md" files into HTML pages is a good choice, because an HTML page is easy to read other than the converted js files. +1 for you.
However there're many complicated things to cope with if you do that.
E.g:
'mdx' has already wrapped some functions of render packages. So do you mean 'mdx' cannot satisfy what you need so you want to remove this reference and change it to 'remark' directly? Nothing worried but just for a confirmation :)
// Need to add the following remark plugins to support GFM and frontmatter | ||
// https://github.com/remarkjs/remark-gfm | ||
// https://github.com/remarkjs/remark-frontmatter | ||
remarkPlugins: [remarkGfm, remarkFrontmatter], |
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.
At the least, I need remarkFrontmatter. format: "md",
makes it treat the markdown as plain markdown (no gfm).
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.
We don't need github flavoured markdown afaik. (Not sure if we use it on our regular build process? And if we do, then yes we need it).
* @see {https://github.com/mdx-js/mdx/discussions/1985} | ||
*/ | ||
export const renderMarkdown = async body => { | ||
const { default: mdx } = await evaluate(body, { |
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.
Where exists renderred version?
post
object just have file path…
{
post: {
title: 'Node v19.4.0 (Current)',
author: 'Rafael Gonzaga',
date: 2023-01-06T13:16:26.671Z,
category: 'release',
slug: '/blog/release/v19.4.0',
file: 'v19.4.0.md'
}
}
{
post: {
title: 'Node v18.13.0 (LTS)',
author: 'Danielle Adams',
date: 2023-01-06T01:01:33.599Z,
category: 'release',
slug: '/blog/release/v18.13.0',
file: 'v18.13.0.md'
}
}
Hey @azu any update here? :) |
I could not found rendered value for post item. nodejs.org/scripts/next-data/getBlogData.mjs Lines 18 to 26 in ec1ebcc
Maybe, It is better that the rss feed should be rendered as a page instead of prerender. https://guillermodlpa.com/blog/how-to-generate-rss-feed-with-next-js Either way, I think it needs a major rewrite, so this PR is closed. |
Hey, @azu could you update the original issue with your findings? We could research and go over this. But I think it was a wise decision to close this PR as any solution would duplicate the processing time (as every blog page would need to be parsed by MDX twice) I super appreciate you diving deep into this issue! |
This PR add "content" to each feed item.
Test Plans
Check local feed reader like https://chrome.google.com/webstore/detail/smart-rss/eggggihfcaabljfpjiiaohloefmgejic
close #5132