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(v2): add meta RSS/Atom feed links to head #2000

Merged
merged 6 commits into from
Nov 30, 2019

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 15, 2019

Motivation

When the blog is enabled, you need to add links to the feeds of this blog.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

See HTML markup.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 15, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 15, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 0bb95bd

https://deploy-preview-2000--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 15, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 0bb95bd

https://deploy-preview-2000--docusaurus-preview.netlify.com

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Although this will be great, this isnt the best way to do it and feels unclean. Finding options from siteconfig.presets is not a bulletproof way. Presets are collection of plugins and it might not even enable feedOptions setting as well. Its also very possible for user to not use preset-classic and just use plugin-content-blog and their own theme component

  • The right way is to actually do this in the blog plugin itself but we are still lacking a “setHead” hooks plugin API

@endiliey
Copy link
Contributor

endiliey commented Nov 15, 2019

by the way feedOptions is optional. If not enabled, no feed at all

export interface PluginOptions {

@lex111
Copy link
Contributor Author

lex111 commented Nov 15, 2019

The right way is to actually do this in the blog plugin itself but we are still lacking a “setHead” hooks plugin API

Fairly enough, I thought about it, but since there was no corresponding API, I decided that as a temporary solution we could go this way (given that the blog plugin is in preset by default, then its functionality will always be there).

BTW, what is the best way to implement a setHead hook?

@endiliey
Copy link
Contributor

endiliey commented Nov 16, 2019

Sorry for that, but I think its better not to provide the temporary solution yet because I am afraid since its the official package, someone will start swizzling and following it.
And blog feed isnt enabled by default for now, so most of the link will be 404 anyway

I think api wise we need to think about the “setHead” (this will help google analytics plugin to set “<link rel preconnect”>) which makes our web perf faster too) and “setBody” (this will solve the dark mode flash too)

First I think we need to design the public API first.

As for implementation, in prod we render the HTML through this https://github.com/facebook/docusaurus/blob/master/packages/docusaurus/src/client/serverEntry.js

In dev server, i think we need to hook to the htmlwebpackplugin.

Summon API guru @yangshun

@endiliey
Copy link
Contributor

Or maybe we just need to somehow connect it here for Head.

https://github.com/facebook/docusaurus/blob/master/packages/docusaurus/src/client/App.js

@lex111
Copy link
Contributor Author

lex111 commented Nov 16, 2019

I agree, perhaps there will not be a quick/easy solution. In addition, I still do not really understand how to implement for example the setHead hook, it is quite difficult for me :(
But I would leave this PR open so that after the corresponding API is implemented, can use it in this PR.

@endiliey endiliey added the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Nov 17, 2019
Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

@endiliey endiliey added pr: new feature This PR adds a new API or behavior. pr: polish This PR adds a very minor behavior improvement that users will enjoy. and removed status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. labels Nov 30, 2019
@lex111
Copy link
Contributor Author

lex111 commented Nov 30, 2019

@endiliey done ✔️, but I'm not sure if this is the correct code, TS typing confuse me 😕

@lex111
Copy link
Contributor Author

lex111 commented Nov 30, 2019

Looks like I'm stuck, an error occurs now :(

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

I fixed it.

@endiliey endiliey merged commit 3caff0d into facebook:master Nov 30, 2019
@endiliey
Copy link
Contributor

Thanks

@lex111
Copy link
Contributor Author

lex111 commented Nov 30, 2019

@endiliey thank you! I am too blunt for TS :|

@lex111 lex111 deleted the meta-feed branch November 30, 2019 16:09
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. pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants