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

fix(v2): allow using classic theme/preset without the docs plugin #3382

Merged
merged 19 commits into from
Sep 1, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Sep 1, 2020

Motivation

Attempt to fix the unability to use the classic theme/preset without the docs plugin
See #3360

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

/blog-only/ deploy preview

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Sep 1, 2020
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 1, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Sep 1, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 14f6bf5

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

@slorber
Copy link
Collaborator Author

slorber commented Sep 1, 2020

Hey @SamChou19815 , this is an attempt to fix #3360

I also added a setup that looks like yours in our deploy previews so that we don't regress on this usecase: https://deploy-preview-3382--docusaurus-2.netlify.app/blog-only/

For classic preset without docs, I just allowed to pass docs: false in config, which probably make more sense in your case than loading uselessly the docs plugin, as you don't want to use it anyway.

I didn't find a good solution for the useDocs hook so for now I also added a dummy file.

Currently, it seems the easy fix has 2 choices:

  • classic preset loads the docs plugin, but allow to have no docs folder
  • classic preset does not load the docs plugin, but we have to fallback in classic theme with the annoying useDocs file.

Do you think one solution is better than the other?
It seems before my refactor we had solution 1, and this PR enables solution 2.
Should we keep using solution 1?

At the same time, solution 2 would allow to use classic theme + blog plugin (like you tried) without using the classic preset, so it looks more flexible to me and allow better composition patterns

@SamChou19815
Copy link
Contributor

Hey @SamChou19815 , this is an attempt to fix #3360

I also added a setup that looks like yours in our deploy previews so that we don't regress on this usecase: https://deploy-preview-3382--docusaurus-2.netlify.app/blog-only/

For classic preset without docs, I just allowed to pass docs: false in config, which probably make more sense in your case than loading uselessly the docs plugin, as you don't want to use it anyway.

I didn't find a good solution for the useDocs hook so for now I also added a dummy file.

Currently, it seems the easy fix has 2 choices:

  • classic preset loads the docs plugin, but allow to have no docs folder
  • classic preset does not load the docs plugin, but we have to fallback in classic theme with the annoying useDocs file.

Do you think one solution is better than the other?
It seems before my refactor we had solution 1, and this PR enables solution 2.
Should we keep using solution 1?

At the same time, solution 2 would allow to use classic theme + blog plugin (like you tried) without using the classic preset, so it looks more flexible to me and allow better composition patterns

I think solution 2 makes more sense. Solution 1 will still suffer from the issue of emitting missing docs folder warning. At least for me, I stayed on preset-classic with docs plugin enabled blog-only mode because it requires less config. The new solutions seem to only require one line of additional config and I think that's fine.

Comment on lines 8 to 15
// See https://github.com/facebook/docusaurus/issues/3360
// TODO find a better solution, this shouldn't be needed
export {};
/*
throw new Error(
"The docs plugin is not used, so you can't require the useDocs hooks. ",
);
*/
Copy link
Contributor

@SamChou19815 SamChou19815 Sep 1, 2020

Choose a reason for hiding this comment

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

We seem to assume here that this dummy theme-classic file is the fallback, and the real hooks in plugin-docs can shadow it. I wonder whether this order of precedence is always desirable. I could imagine another potential use case that requires the opposite order of precedence: plugin-debug defines some default components, and let theme-classic or theme-bootstrap to provide better styling. Although I think the plugin-debug use case is probably less important.

Copy link
Collaborator Author

@slorber slorber Sep 1, 2020

Choose a reason for hiding this comment

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

Thanks for your insights :) currently trying to figure this out by using the @theme-init alias and see if I can "forward" real useDocs impl conditionnnally, if docs plugin is available. Not sure how to do this properly, Webpack emits a warning on conditional requires (webpack/webpack#7713 (comment))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems to work according to the tests but there is an annoying webpack warning, not sure how to improve this.

@slorber
Copy link
Collaborator Author

slorber commented Sep 1, 2020

Thanks for your insight @SamChou19815

I suggest we merge this PR, even if it produces an annoying webpack warning it seems to work fine for solution2.

The problem is that we currently don't have a good way for a plugin to expose client side utils like useDocs.

This code should be provided by the plugin because it is useful for usage in multiple themes (like classic/bootstrap), but I don't think using @theme in plugin is the best solution for that (we loose typesafety by not emitting typedefs btw), so I'm not sure it's worth investing too much in a solution that would be changed later anyway.

@slorber
Copy link
Collaborator Author

slorber commented Sep 1, 2020

Basically this shows this webpack error on build:

image

webpack/webpack#7713 (comment)

I was actually able to remove that warning using warningsFilter webpack option (not sure why but we seem to reimplement it in Docusaurus btw): https://webpack.js.org/configuration/stats/#statswarningsfilter

@slorber slorber closed this Sep 1, 2020
@slorber slorber reopened this Sep 1, 2020
@slorber slorber merged commit 5359d61 into master Sep 1, 2020
@slorber slorber deleted the slorber/blogOnly branch August 17, 2021 18:00
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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants