-
-
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): Extract npm2yarn plugin #3469
Conversation
Deploy preview for docusaurus-2 ready! Built with commit 0a98a6e |
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.
Not sure if we really should include this feature in the core. This is a pretty specific thing. 🤔
Wouldn't it be better if this is published as an npm package outside of Docusaurus core?
The feature of tabs is ours. I don't know if it's still valuable outside. |
Not exactly, because the tabs are feature of a theme, not the core. For this reason, it is best to place this tabs extension in a separate npm package. |
Like a docusaurus plugin outside of the core, right? But why? I'm trying to understand your idea |
Like a docusaurus plugin outside of the core, right?
Yep, it is sort of a separation of concerns, plus we only ship what is
really need (basic things).
2020-09-22 0:49 GMT+03:00, Fanny <notifications@github.com>:
… Like a docusaurus plugin outside of the core, right?
But why? I'm trying to understand your idea
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#3469 (comment)
|
Sounds good. I'll wait @slorber return from PTO, to receive a final feedback. |
As we are based on JS, React, MDX etc, it's probably ok to include features slightly biased to JS usage, as many docusaurus sites document frontend projects, as long as it does not harm or add too much clutter for other documentation usecases. This usecase seems fine to me. Maintaining a plugin that we use on Docusaurus site outside of Docusaurus repo can be annoying, as it adds friction for maintenance and refactorings, particularly if the owner (Fanny?) becomes inactive for some reason. @lex111 is right that tabs is a theme feature, so adding this plugin by default in core (I mean mdx loader) seems bad. We are lucky that both classic and bootstrap themes support tabs so CI does not fail, but if community builds a theme without tabs, this plugin would make the site fail. Maybe we should add the ability for a theme to register global remark/rehype plugins? Not sure how this can be done properly. However this feature is already opt-in due to its specific syntax What I suggest is that we publish the plugin as Later, we could look for a way to enable this plugin by default on classic/bootstrap presets/themes, as we know in such case that it is safe to use. What do you think? |
I agree with you, I didn't think in the possibility that the tabs feature couldn't used. That said, this option, seems secure for me. |
Hi @fanny Do you want to work on the changes we agreed on, or can I finish and merge your PR? |
Hey, I can finish. I was waiting for @lex111 comments. |
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.
Almost good, just an important details 🤪 this does not work.
You didn't actually use the plugin you just created, because you still use the local site plugin in the config:
remarkPlugins: [require('./src/plugins/remark-npm2yarn')],
We should remove this local plugin, and change the config to use the new remark plugin:
remarkPlugins: ["@docusaurus/remark-plugin-npm2yarn"],
(maybe require() is still required, don't know)
Without a new lifecycle like getRemarkPlugins
or something, we currently don't have a way to make a docusaurus plugin provide a remark plugin that will be used for all other plugins reading markdown files.
I suggest to keep it simple for now (and publish a remark plugin, not a docusaurus plugin), but in the future we'd rather figure out such an API, so that adding "@docusaurus/plugin-npm2yarn" would enable the npm2yarn feature for all plugins (docs, pages, blog...).
return { | ||
name: 'docusaurus-remark-plugin-npm2yarn', | ||
getClientModules() { | ||
return [path.resolve(__dirname, './npm2yarn/index.js')]; |
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 don't think this will work in practice.
We currently don't have any lifecycle method to add a remark plugin to the mdx loader in a generic way.
For now, this package should be a simple remark plugin, not a docusaurus plugin (until we figure this out). We'll use it in the conf by providing it in the remark plugin array.
"license": "MIT", | ||
"dependencies": { | ||
"npm-to-yarn": "^1.0.0-2", | ||
"@docusaurus/mdx-loader": "2.0.0-alpha.65" |
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.
probably not needed
Ops, I understood wrong, so in this case, could you finish? I don't know if I will have time |
sure, will do that :) |
ready to merge, thanks ;) |
@@ -17,6 +17,7 @@ | |||
"@docusaurus/utils": "2.0.0-alpha.65", | |||
"@mdx-js/mdx": "^1.5.8", | |||
"@mdx-js/react": "^1.5.8", | |||
"npm-to-yarn": "^1.0.0-2", |
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 really needed for docusaurus-mdx-loader ?
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, not needed :)
do you want to submit a PR?
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.
Sure
Motivation
I would like to extract our npm2yarn plugin to the mdx loader. Why am I doing this? A couple of months ago, I tried to implement the plugin in CRA they demonstrated interest in use the plugin, and other users like react-navigation are using as well. But doing as a copy-paste version from ours.
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!)
yarn test
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.)