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): option and config validation life cycle method for official plugins #2943

Merged
merged 25 commits into from
Jun 24, 2020

Conversation

anshulrgoyal
Copy link
Contributor

Motivation

All the plugins don't validate and normalize options. It tries to generalize option validation and normalization.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

By adding tests for different plugins validation and normalization.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 16, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jun 16, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 408d871

https://deploy-preview-2943--docusaurus-2.netlify.app

@anshulrgoyal anshulrgoyal reopened this Jun 17, 2020
@anshulrgoyal anshulrgoyal marked this pull request as ready for review June 17, 2020 15:23
@anshulrgoyal anshulrgoyal requested a review from yangshun as a code owner June 17, 2020 15:23
@anshulrgoyal anshulrgoyal requested a review from lex111 as a code owner June 18, 2020 06:47
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.

That looks like a great start!


const getFeedTypes = (type?: FeedType) => {
assertFeedTypes(type);
const getFeedTypes = (type: FeedType) => {
let feedTypes: ('rss' | 'atom')[] = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Omit<FeedType,"all"> would be more future proof. Wonder if we shouldn't simply remove this "all" from the type, as it's somehow a "shortcut"

packages/docusaurus-theme-classic/src/index.js Outdated Show resolved Hide resolved
packages/docusaurus-types/src/index.d.ts Outdated Show resolved Hide resolved
packages/docusaurus-plugin-content-blog/src/types.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/server/plugins/init.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/server/plugins/init.ts Outdated Show resolved Hide resolved

module.exports.validateThemeConfig = ({validate, themeConfig}) => {
return validate(ThemeConfigSchema, themeConfig);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

this assigns the validateThemeConfig to the default exported plugin function.

I'm not very fan of this idea, but this is a bit subjective (api design choices).

I think we have 2 cases here:

    1. compiled plugins (ie in this repo, the ones in TS), where we can use ES imports/exports
    1. "raw" plugins (like this one, and maybe the ones the user will write), for which we can't use ES imports/exports, otherwise we wouldn't support older nodejs versions

What about exporting these static methods directly?

  1. => export function validateThemeConfig() {};

  2. => exports.validateThemeConfig = function() {};

It could even be possible to support both syntaxes 🤪 wonder the one we should recommend to the users

@anshulrgoyal anshulrgoyal requested a review from slorber June 22, 2020 08:36
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.

That seems nice, just a few things to discuss/change before merging ;)

@anshulrgoyal anshulrgoyal requested a review from slorber June 22, 2020 13:29
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.

LGTM

Just 2 things left:

  • Support modern ES syntax
  • Some documentation

themeConfig: context.siteConfig.themeConfig,
});
}
return plugin(context, pluginOptions);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should support plain ES export syntax here:

export function validateThemeConfig() { }

It does not look like it would work.

We'd need something like:

const validateOptions = pluginModule.default?. validateOptions ?? pluginModule.validateOptions;
if ( validateOptions ) {
  ...
}

For both plugins and themes.

We could even migrate the existing plugin to ES as well.

Note: we are recommending node >= 10 (not supporting ES imports), so only the TS/transpiled plugins can be migrated.

We should rather document both syntaxes. ES is more modern, but requires newest node versions of 12/14, while Common-js would work as well on older node versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For documentation, I will add lifecycle methods to docs. And for import, I think we can support both.

@anshulrgoyal anshulrgoyal requested a review from slorber June 24, 2020 05:13
@slorber slorber merged commit 81d8553 into facebook:master Jun 24, 2020
@slorber
Copy link
Collaborator

slorber commented Jun 24, 2020

awesome, let's finally merge this!

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Jun 24, 2020
export const DefaultOptions = {
feedOptions: {},
beforeDefaultRehypePlugins: [],
beforeDefaultRemarkPlugins: [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, are these options a typo?

That looks like, please confirm so that I can remove in my current pr

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah no it seems used in practice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied it from docs.https://v2.docusaurus.io/docs/using-plugins/#docusaurusplugin-content-blog

        beforeDefaultRemarkPlugins: [],
        beforeDefaultRehypePlugins: [],
        feedOptions: {
          type: '', // required. 'rss' | 'feed' | 'all'
          title: '', // default to siteConfig.title
          description: '', // default to  `${siteConfig.title} Blog`
          copyright: '',
          language: undefined, // possible values: http://www.w3.org/TR/REC-html40/struct/dirlang.html#langcodes
        },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah no it seems used in practice

Sorry I didn't understand what u mean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nevermind I thought it was not used but it is

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.

4 participants