Content plugins should prevent usage of a parent folder as mdx source path

slorber commented 1 year ago

Have you read the Contributing Guidelines on issues?

[X] I have read the Contributing Guidelines on issues.

Motivation

Some users attempt to do:

presets: [
    [
      "classic",
      ({
        docs: {
          path: "..",
        // ...
      }),
    ],
  ],

See https://github.com/facebook/docusaurus/discussions/9002

This is bad because using the site root (or any of its parent) as the source of md/mdx content will register a mdx loader for that folder, and lead to conflicts with the blog/page plugins that are nested inside that folder.

The error message is not very clear, such as:

[ERROR] MDX loader can't read MDX metadata file "/home/md/github/design-docs/docs-website/.docusaurus/docusaurus-plugin-content-docs/default/site-src-pages-markdown-page-md-393.json". Maybe the isMDXPartial option function was not provided?

I think we should improve the DX and fail-fast, forbidding the use of the site folder (or any of its parent) as content (md/mdx) source root.

Eventually the error message could mention that if user wants to have some content folders upper in the tree (like it was in Docusaurus v1: ../docs), it's possible to use that specific folder inside a parent dir. It can be useful to also mention multi-instance as an option (see https://github.com/facebook/docusaurus/discussions/9002#discussioncomment-6048315)

Technically it might be possible to order mdx-loaders in an order that could make it work, but I doubt it's a good idea to allow that anyway and could lead to more annoying problems too, for example users trying to use autogenerated docs but it picks content from blog/pages too... So I think it's safer to just forbid this entirely.

To generalize: the problem is to have one content folder nested inside another. Unfortunately due to the modular nature of Docusaurus (the concept of a "content folder" is plugin-specific), it's probably not possible to generalize the solution and we can only prevent he most common case: declaring a direct site parent folder as content folder.

Self-service

[X] I'd be willing to do some initial work on this proposal myself.

enzofrnt commented 1 month ago

I don't think we should restrict users as you suggested. I believe it should be possible in certain cases (like mine currently). It can be useful to reuse the original documentation that was used in a project. For example, a company might have built their documentation starting from the README and other folders in their Git repo. If they want to add Docusaurus and use the README as the Docusaurus intro, they wouldn't be able to do so.

I'm pretty sure this is a bad idea, because sometimes Docusaurus can be used simply to "beautify" the documentation that was originally done in the Git tree. It's important to allow this flexibility so as not to hinder users who want to leverage Docusaurus to enhance their existing documentation.

Josh-Cena commented 1 month ago

@enzofrnt: consider this pattern: https://github.com/facebook/docusaurus/blob/main/website/community/3-contributing.mdx?plain=1

slorber commented 1 month ago

Something worth mentioning is that Docusaurus uses MDX, and MDX compiles Markdown documents to React components.

For this reason, the MDX React components must be able to find the require node_module dependencies in a parent folder: we don't have full freedom to put the Markdown files anywhere we want due to that limitation.

We do have a fallback to support rendering <root>/website/README.md as MDX. See what @Josh-Cena suggested: not by declaring <root>/website as docs routeBasePath but by importing the <root>/website/README.md in another doc such as <root>/website/docs/myDoc.mdx).

But you shouldn't try to render <root>/README.md in Docusaurus (unless you use a monorepo hoisting node_modules dependencies at the root 🤷‍♂️ ). It's simpler/preferable that you put all the docs inside the <root>/website folder, and if you choose not to follow that recommendation you'd better know what you are doing and understand how MDX and Node.js module resolution works.

Do you want the freedom to render any md file from anywhere in your filesystem? You will have to use Webpack raw-loader (importing md content as pure strings instead of React components) and then use a runtime Markdown library to import/render that raw string inside other MDX docs. Or create your own plugin that's not based on MDX.

# This is an MDX doc in `website/docs`

import RenderMarkdown from 'runtime-markdown-lib';
import ImportedMarkdownDocString from '!!raw-loader!/anywhere/in/my/filesystem.md';

<RenderMarkdown>{MyComponentSource}</RenderMarkdown>

Hope this makes sense.

alirezamirian commented 1 month ago

Something worth mentioning is that Docusaurus uses MDX, and MDX compiles Markdown documents to React components.

For this reason, the MDX React components must be able to find the require node_module dependencies in a parent folder: we don't have full freedom to put the Markdown files anywhere we want due to that limitation.

TL;DR

The issue is not as generic. It's only about react and react-dom. Aliasing react and react-dom to whatever path they resolve from website root prevents potential issues and is a sensible configuration to have by default.

Yes, MDX files are compiled into a js code that imports react, and for things to work, those imports to react must resolve to the same location that react resolves inside the app. But isn't that the limit? Consider a case like this:

.
├── website/  # (docusaurus)
│   └── node_modules/
├── docs/ # (docs folder)
│   └── doc.mdx
└── node_modules/

As you mentioned too, it works if dependencies are hoisted. But more specifically, it works if react is hoisted, which makes react resolve to the same location in these two cases:

when imported from the app
when imported from the compiled mdx file (or more generically, any module that imports react and is imported in the app)

Which is basically a requirement because of which react and react-dom are peer dependencies. But the difference in case of mdx files is that the import to react is in a generated code (the react component the mdx content compiles to), as opposed to imports to react from components in npm packages, where the peer dependency requirement is ensured by the package manager.

A solution is to add an alias for react and react-dom (not coincidently peer dependencies of @docusaurus/core!), in the webpack configuration, to have them resolve to the same location they would resolve when imported by the app:

{
  "resolve": {
    "alias": {
     // The trick to use "/package.json" is because require.resolve would resolve to the exact js file, but we want the path to the directory where react is installed.
      react: path.dirname(require.resolve("react/package.json")),
      "react-dom": path.dirname(require.resolve("react-dom/package.json")),
    }
   }
}

Here is a full example of how disabling hoisting breaks things and how the aliasing solution fixes it: https://codesandbox.io/p/devbox/charming-river-x776sj

slorber commented 1 month ago

@alirezamirian that looks like a reasonable suggestion.

I've implemented it here with a secret env variable to opt-out just in case it cause troubles to our users for unexpected reasons: https://github.com/facebook/docusaurus/pull/10391

Note: I'm keeping this issue open because we should still forbid users to have one docs folder nested into another.

I also doubt using the site root as docs plugin source folder is a good idea.

The aliasing suggestion is mostly useful to solve problems where docs are outside the website folder / monorepo.

slorber commented 1 month ago

Note we also need to alias @mdx-js/react because MDX will eventually add an extra import depending on the MDX content to import components from React context.

import {useMDXComponents as _provideComponents} from '@mdx-js/react'

Note you can also implement the solution yourself in userland:

function plugin() {
  return {
    name: "my-plugin",
    configureWebpack() {
      return {
        resolve: {
          alias: {
            // The trick to use "/package.json" is because require.resolve would resolve to the exact js file, but we want the path to the directory where react is installed.
            react: path.dirname(require.resolve("react/package.json")),
            "react-dom": path.dirname(
              require.resolve("react-dom/package.json"),
            ),
            "@mdx-js/react": path.dirname(require.resolve("@mdx-js/react")),
          },
        },
      };
    },
  };
}

alirezamirian commented 1 month ago

Awesome! Thanks!

Note: I'm keeping this issue open because we should still forbid users to have one docs folder nested into another.

I also doubt using the site root as docs plugin source folder is a good idea.

The aliasing suggestion is mostly useful to solve problems where docs are outside the website folder / monorepo.

Yes, exactly, the aliasing is relevant for docs folder outside website, which is a superset of docs folder being a parent folder.

I actually have some observations and suggestions regarding the particular case of a parent folder being set as docs root. I'm planning to write here later. I hope I make it in time before you officially forbid that :D

alirezamirian commented 1 month ago

Note we also need to alias @mdx-js/react because MDX will eventually add an extra import depending on the MDX content to import components from React context.

True. In fact I have done that already in my actual project, but had forgotten to update my comment :)

facebook / docusaurus