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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"scripts:release-post": "node scripts/release-post/index.mjs"
},
"dependencies": {
"@mdx-js/mdx": "^2.3.0",
"@mdx-js/react": "^2.3.0",
"classnames": "^2.3.2",
"feed": "^4.2.2",
Expand All @@ -43,6 +44,8 @@
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-intl": "^6.2.10",
"remark-frontmatter": "^4.0.1",
"remark-gfm": "^3.0.1",
"sass": "^1.59.2",
"semver": "^7.3.8",
"strftime": "^0.10.1",
Expand Down
41 changes: 35 additions & 6 deletions scripts/next-data/generatePreBuildFiles.mjs
Original file line number Diff line number Diff line change
@@ -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 { join } from 'path';
import { Feed } from 'feed';

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.

import remarkFrontmatter from 'remark-frontmatter'
import * as runtime from 'react/jsx-runtime'
import { getRelativePath } from './_helpers.mjs';

// imports the global site config
Expand All @@ -29,6 +34,25 @@ export const generateBlogYearPages = cachedBlogData =>
.then(data => [...new Set(data)])
.then(data => data.forEach(createBlogYearFile));

/**
* 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, {
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.

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

})
return renderToString(createElement(mdx))
}
Comment on lines +37 to +55
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 :)

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?

export const generateWebsiteFeeds = cachedBlogData =>
siteConfig.rssFeeds.forEach(metadata => {
const feed = new Feed({
Expand All @@ -43,16 +67,21 @@ export const generateWebsiteFeeds = cachedBlogData =>
? `/en/blog/${metadata.blogCategory}`
: '/en/blog';

const mapBlogPostToFeed = post =>
feed.addItem({
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

const htmlContent = await renderMarkdown(markdownContent)
return {
title: post.title,
content: htmlContent,
id: `https://nodejs.org/en${post.slug}`,
link: `https://nodejs.org/en${post.slug}`,
author: post.author,
date: new Date(post.date),
});
};
};

cachedBlogData(blogCategoryOrAll)
.then(({ blogData }) => blogData.posts.forEach(mapBlogPostToFeed))
.then(({ blogData }) => Promise.all(blogData.posts.map(mapBlogPostToFeed)))
.then((feedItems) => feedItems.forEach(feedItem => feed.addItem(feedItem)))
.then(() => writeFile(join(publicFeedPath, metadata.file), feed.rss2()));
});