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(content-blog): add full blog post html into RSS/Atom feeds #4330

Merged
merged 16 commits into from
Oct 8, 2021

Conversation

moonrailgun
Copy link
Contributor

Motivation

RSS Reader should get full content

Have you read the Contributing Guidelines on pull requests?

YEP

Test Plan

Just take a look of website rss xml file. and check item content

Further Plan

If accept this pr, i know my way is right and i will try to translate origin mdx to html, for make rss user have better experiences.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 3, 2021
@netlify
Copy link

netlify bot commented Mar 3, 2021

@github-actions
Copy link

github-actions bot commented Mar 3, 2021

Size Change: -46 B (0%)

Total Size: 532 kB

Filename Size Change
website/build/assets/css/styles.********.css 87.1 kB -46 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/build/assets/js/main.********.js 359 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 60.3 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 25.4 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Mar 3, 2021

✔️ [V2]

🔨 Explore the source changes: c4d84b1

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61601d90517c5c000882b8d3

😎 Browse the preview: https://deploy-preview-4330--docusaurus-2.netlify.app

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR, this has been requested here already: #3719

Your implementation is not correct because it assume the blog post content is a simple string, which is not the case in practice as we use MDX

Read my comment here. on how you could transform an MDX string to an HTML string:

#3719 (comment)

@slorber slorber linked an issue Mar 3, 2021 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Mar 5, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 100
🟠 Accessibility 72
🟢 Best practices 93
🟠 SEO 50
🟠 PWA 55

Lighthouse ran on https://deploy-preview-4330--docusaurus-2.netlify.app/

@moonrailgun
Copy link
Contributor Author

Hi @slorber , Thanks for your review and reply.

First commit is just backup to ensure the full blog rss content is expected behaviour

I update content generate logic to make sure rss reader can get a readable, plain html string. So i remove import statement with remark-mdx-remove-imports, although this package marked as Deprecated but its still have a nice work.

And update testcase to ensure bigger cover range.

Before:
image

After:
image

@moonrailgun moonrailgun requested a review from slorber March 5, 2021 05:58
@moonrailgun
Copy link
Contributor Author

Is any other problem for this pr?

@MatanBobi
Copy link
Contributor

I update content generate logic to make sure rss reader can get a readable, plain html string. So i remove import statement with remark-mdx-remove-imports, although this package marked as Deprecated but its still have a nice work.

I'm not sure I understand why we need remark-mdx-remove-imports, can you please explain? :)
Also, if that package is considered deprecated, IMO we should not use it because if it will have bugs, there won't be any support and we won't be able to fix those.

Regarding the PR, I'm pretty sure React and React-DOM should be defined as peerDependencies and not dependencies. Also, I think the mdx-runtime functionality should be a part of mdx-loader and not the docusaurus-utils package.

CC: @slorber

@moonrailgun
Copy link
Contributor Author

I'm not sure I understand why we need remark-mdx-remove-imports, can you please explain? :)

because rss content should be a plain html, and in most custom component, its have their own javascript logic.

Its will not a nice work in feed reader.

Its just my mind

@moonrailgun
Copy link
Contributor Author

Regarding the PR, I'm pretty sure React and React-DOM should be defined as peerDependencies and not dependencies. Also, I think the mdx-runtime functionality should be a part of mdx-loader and not the docusaurus-utils package.

Hi @MatanBobi , thanks for your review, i am move those into peerDependencies, its my mistake.

And for mdx-loader, this package worked as a webpack loader, how can i use in feed generate? I have no idea about it. Could you explain more about your idea?

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 7, 2021
# Conflicts:
#	packages/docusaurus-plugin-content-blog/src/__tests__/__snapshots__/generateBlogFeed.test.ts.snap
#	packages/docusaurus-plugin-content-blog/src/__tests__/index.test.ts
#	packages/docusaurus-plugin-content-blog/src/blogUtils.ts
#	packages/docusaurus-utils/package.json
#	yarn.lock
@slorber
Copy link
Collaborator

slorber commented Oct 8, 2021

Hey.

After more tests, I'm finally merging this PR, as I think it is better than before and good enough for now.

Keep in mind that the RSS feed content has some limitations that I documented here: #5664

@slorber slorber changed the title feat(v2): add full content into feeds feat(content-blog): add full blog post html into RSS & Atom feeds Oct 8, 2021
@slorber slorber changed the title feat(content-blog): add full blog post html into RSS & Atom feeds feat(content-blog): add full blog post html into RSS/Atom feeds Oct 8, 2021
@slorber slorber merged commit 6ed6989 into facebook:main Oct 8, 2021
@moonrailgun
Copy link
Contributor Author

Nice! Thanks for your code and time!

RSS feature is soooo awesome for a RSS reader!

@slorber
Copy link
Collaborator

slorber commented Oct 20, 2021

Note, if you use require() calls in your mdx blog posts, your site build will fail and it is annoying for some users.

In #5753 I temporarily swallow such errors, so if you have a blog post that doesn't render fully in the feed, it means there's an issue and we couldn't render it.

See also #5664 for explainations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V2: Non truncated blog article in feeds
4 participants