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

V2: Enable feeds by default in blog plugin #3720

Closed
nickserv opened this issue Nov 10, 2020 · 11 comments · Fixed by #3842
Closed

V2: Enable feeds by default in blog plugin #3720

nickserv opened this issue Nov 10, 2020 · 11 comments · Fixed by #3842
Labels
feature This is not a bug or issue with Docusausus, per se. It is a feature request for the future. good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. help wanted Asking for outside help and/or contributions to this particular issue or PR.

Comments

@nickserv
Copy link
Contributor

🚀 Feature

The blog plugin should enable feeds by default.

Motivation

Docusaurus 1 enables both blogs and feeds by default. Docusaurus 2 extracts the blog into a separate plugin so it makes sense that blogs aren't enabled by default, however I would expect feeds to also be enabled by default with the blog plugin.

Additionally, feeds would be useful to most users as it's unlikely that blogs would be monetized for commercial purposes (see #3719 for more rationale).

Pitch

The blog plugin should behave as if feedOptions: { type: 'all' } was in the configuration by default

@nickserv nickserv added feature This is not a bug or issue with Docusausus, per se. It is a feature request for the future. status: needs triage This issue has not been triaged by maintainers labels Nov 10, 2020
@slorber slorber added good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. help wanted Asking for outside help and/or contributions to this particular issue or PR. and removed status: needs triage This issue has not been triaged by maintainers labels Nov 12, 2020
@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

Why not having a feed by default, but it's a breaking change.

Good first issue

@cindygu4
Copy link
Contributor

Hi, I'm interested in taking on this feature if that's okay :)

@slorber
Copy link
Collaborator

slorber commented Nov 12, 2020

yes thanks

@vikhramt007
Copy link

Me and @cindygu4 are working on this feature.
We're new to Docusaurus, so we just wanted to ask what enabling feeds by default looks like and how we can test it out.

@nickserv
Copy link
Contributor Author

nickserv commented Nov 19, 2020

@vikhramt007 @cindygu4

what enabling feeds by default looks like

I believe you can implement this by changing the Joi type validation schema to use 'all' (which produces ['rss', 'atom'] in the internal config) by default.

how we can test it out

Generating the site should get you /blog/rss.xml and /blog/atom.xml files by default.

@slorber
Copy link
Collaborator

slorber commented Nov 19, 2020

yes changing the Joi default value could be enough.

you can test it with unit tests: missing blog feed config leads to rss+atom
you can test it locally with yarn build:v2 and checking files in website/build as explained by @nickmccurdy

@vikhramt007
Copy link

yes changing the Joi default value could be enough.

you can test it with unit tests: missing blog feed config leads to rss+atom
you can test it locally with yarn build:v2 and checking files in website/build as explained by @nickmccurdy

@slorber
Hi, we started to work on this feature, and we would like some clarification on whether "missing blog feed configuration" means
feedOptions: {type: '', title: 'myblog'} or feedOptions: {title: 'myblog'} ?

If its the former, how would we change feedOptions.type so we don't receive any validation errors? If its the latter, how would we approach this?
Thanks

@cindygu4
Copy link
Contributor

Hi @slorber,

@vikhramt007 and I wrote a unit test for this feature as shown below:
Screen Shot 2020-11-25 at 3 11 28 PM

We ended up doing so that type accepts an empty string parameter, {type: ''}. This is because we found that changing the Joi default value led to errors in other tests. Right now, our unit test case passes along with all the other unit tests. However, when we try to test it locally using yarn build:v2, we get a validation error telling us '"feedOptions.type" does not match any of the allowed types.' Are you aware of how {type: 'all'} is resolved since we didn't find it in docusaurus-plugin-content-blog/src/types.ts and we only foundexport type FeedType = 'rss' | 'atom';.

Thanks!

@slorber
Copy link
Collaborator

slorber commented Nov 26, 2020

Hi,

It would be easier for me to understand what's your problem if you had a draft PR opened where I could inspect a code diff

"" is not really a value we should accept

We should allow type: null undefined (= type: "all")
We should allow feedOptions: null, feedOptions: {title: "title"}

If other tests say something else, you should change these other tests so that they conform to what we actually want.

Are you aware of how {type: 'all'} is resolved since we didn't find it in docusaurus-plugin-content-blog/src/types.ts and we only foundexport type FeedType = 'rss' | 'atom';.

Afaik it's only a shortcut in the schema, after normalization of options, you can't end up with a type: "all" code, so it's only in the Joi declaration file:

    type: Joi.alternatives().conditional(
      Joi.string().equal('all', 'rss', 'atom'),
      {
        then: Joi.custom((val) => (val === 'all' ? ['rss', 'atom'] : [val])),
      },
    ),

I merged this morning many changes so you'd rather update your branch asap to sync with recent code.

@cindygu4
Copy link
Contributor

Hi @slorber,

Thank you for your help! It really helped clear up our understanding. We've been able to implement this feature and made a pull request a little bit ago :)

@slorber slorber linked a pull request Dec 10, 2020 that will close this issue
@slorber
Copy link
Collaborator

slorber commented Dec 10, 2020

closed by #3842

@slorber slorber closed this as completed Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This is not a bug or issue with Docusausus, per se. It is a feature request for the future. good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. help wanted Asking for outside help and/or contributions to this particular issue or PR.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants