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

feat: support content on RSS Feeds #5144

Closed
wants to merge 2 commits into from
Closed

feat: support content on RSS Feeds #5144

wants to merge 2 commits into from

Conversation

azu
Copy link

@azu azu commented Mar 15, 2023

This PR add "content" to each feed item.

Test Plans

Check local feed reader like https://chrome.google.com/webstore/detail/smart-rss/eggggihfcaabljfpjiiaohloefmgejic

image

close #5132

* @see {https://github.com/mdx-js/mdx/discussions/1985}
*/
export const renderMarkdown = async body => {
const { default: mdx } = await evaluate(body, {
Copy link
Member

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?

Copy link
Author

@azu azu Mar 15, 2023

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

Copy link
Member

@ovflowd ovflowd Mar 15, 2023

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.

Copy link
Member

@ovflowd ovflowd left a 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';
Copy link
Member

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

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?

Copy link
Member

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'
Copy link
Member

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))
}
Copy link
Member

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');
Copy link
Member

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

Comment on lines +37 to +55
/**
* 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))
}
Copy link
Author

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.

Copy link
Contributor

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],
Copy link
Author

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).

Copy link
Member

@ovflowd ovflowd Mar 15, 2023

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, {
Copy link
Author

@azu azu Mar 15, 2023

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

@ovflowd
Copy link
Member

ovflowd commented Mar 20, 2023

Hey @azu any update here? :)

@azu
Copy link
Author

azu commented Mar 21, 2023

I could not found rendered value for post item.
post is here, but the content is markdown.

const getMatter = name => content => {
const { title, author, date, category } = graymatter(content).data;
// we only want the actual name of the file without the extension
// and then prepend the category name to it
const slug = `/blog/${category}/${basename(name, extname(name))}`;
return { title, author, date, category, slug, file: basename(name) };
};

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
https://zenn.dev/catnose99/articles/c7754ba6e4adac
A page component can render rss feed(xml) instead of html page(It is a bit hacky).
Probably, page component can receive rendered markdown and just use it.
(It should also work well with watch mode.)
I am not familiar with nextra, so I don't know if this can be achieved.

Either way, I think it needs a major rewrite, so this PR is closed.

@azu azu closed this Mar 21, 2023
@ovflowd
Copy link
Member

ovflowd commented Mar 21, 2023

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RSS feed is missing description/article content for posts
4 participants