-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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): allow config plugins as functions or [function,options] #4618
Conversation
[V2] Built with commit 0c8e545 |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4618--docusaurus-2.netlify.app/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that looks like a good start.
ESLint is not happy and report some lint errors to fix + added a few inline comments.
I find this code starts to be a bit hard to follow, do you think we could refactor it to something simpler? My idea is to follow this recommendation and avoid using "let" reassignments: https://charles-stover.medium.com/replacing-let-with-const-86797b790775
Also we need some doc for this here: https://docusaurus.io/docs/next/using-plugins#creating-plugins
I'd like this function plugin to be the highlighted way for users to create a plugin.
@@ -153,6 +139,16 @@ describe('normalizeConfig', () => { | |||
['this/should/work', {too: 'yes'}], | |||
], | |||
], | |||
[ | |||
'should accept function for plugin', [ | |||
function(context, params) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function(context, params) {} | |
function(_context, _options) {} |
_ permits to make eslint happy if you don't need to use the param
options is used instead of params
packages/docusaurus/src/server/plugins/__tests__/__fixtures__/simple-site/docs/hello.md
Outdated
Show resolved
Hide resolved
packages/docusaurus/src/server/plugins/__tests__/__fixtures__/simple-site/docusaurus.config.js
Outdated
Show resolved
Hide resolved
packages/docusaurus/src/server/__tests__/configValidation.test.ts
Outdated
Show resolved
Hide resolved
packages/docusaurus/src/server/plugins/__tests__/__fixtures__/simple-site/docusaurus.config.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that starts to look good.
Added a few comments + a commit to fix some issues on the PR
break; | ||
} else { | ||
packagePath = path.dirname(packagePath); | ||
return plugins |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a hard time understanding what this code is doing 😅 can you explain your changes please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getPluginNames() function fetches all module plugins and gets its name through its package.json. This was working fine because the signature for PluginConfig used to be just [string, {}], [string] or string. Since I added functional plugin support, these don't have a package.json file so I'm filtering for only plugins which use a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see thanks. Don't hesitate to add comments or try to create smaller functions with good names, so that the code becomes self-documented. I'm not sure I'll understand this code again in 6 months 😅
...s/docusaurus/src/server/plugins/__tests__/__fixtures__/site-with-plugin/docusaurus.config.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,5 @@ | |||
module.exports = function (context, options) { | |||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usually intend with 2 spaces, not sure why prettier does not complain about this
|
||
import path from 'path'; | ||
|
||
import {loadContext} from '@docusaurus/core/src/server/index'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the other part of the issue: code from core should use relative imports, not code from @docusaurus/core
that is the public API for other packages that depends on core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import {loadContext} from '../../index';
````js title="docusaurus.config.js" | ||
module.exports = { | ||
// ... | ||
plugins: [function(contex, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some line-breaks here and. add // highlight-start
and // highlight-end
around the plugin function
return { | ||
name: 'my-docusaurus-plugin', | ||
async loadContent() { | ||
/* ... */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* ... */ | |
// ... |
For consistency with the pattern used above?
@@ -166,7 +190,7 @@ interface LoadContext { | |||
outDir: string; | |||
baseUrl: string; | |||
} | |||
``` | |||
```` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed?
if (!pluginModuleImport) { | ||
const pluginModuleImport = getPluginModuleImport(); | ||
const plugin = getPlugin(); | ||
pluginOptions = getPluginOptions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not const? I don't think options need reassignment
return null; | ||
} | ||
|
||
function getPluginModuleImport(): string | null { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you extract all those fns outside of initPlugins? I think it would help make the code more readable by passing the required data as function params (make the fn dependencies more explicit)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions being inside a function where they are only used was common in docusaurus-plugin-content-docs so I thought I would follow that pattern since these functions are not used anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functions being inside a function where they are only used was common in docusaurus-plugin-content-docs so I thought I would follow that pattern since these functions are not used anywhere else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a big deal but splitting as much as possible into multiple pure functions makes the code more explicit, readable and testable.
Historically, most of the code of the docs plugin was directly inside index.test.ts, and I moved code outside of it progressively.
Do your best, I'll continue the refactor later if you don't find a solution to extract those methods, it's not high priority and shouldn't block merging this PR.
[V1] Built with commit 0c8e545 |
return pluginItem; | ||
} | ||
|
||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this case actually possible (assuming we have a good validation?)
If it's not possible, then throw an error like throw new Error("Unexpected")
.
In general it's a good practice to fail-fast in case of unexpected met, like at the end/default of a switch/case or in the "else" branch of conditionals.
Didn't read but looks like a good article to illustrate the concept: https://levelup.gitconnected.com/switch-guard-in-typescript-how-to-make-sure-all-possible-cases-are-handled-19e86fd15797
Thanks @besemuna , LGTM Was able to cleanup the plugin init code, looks easier to understand now |
Motivation
This is an attempt ti fix #3934
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)
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.)