Breaking change in v2.2.0 of MDX Loader

[sean-perkins]

Have you read the Contributing Guidelines on issues?

• I have read the Contributing Guidelines on issues.

Prerequisites

• I'm using the latest version of Docusaurus.
• I have tried the npm run clear or yarn clear command.
• I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
• I have tried creating a repro with https://new.docusaurus.io.
• I have read the console error message carefully (if applicable).

Description

The 2.2.0 release for Mermaid introduced a breaking change to the type for Options in @docusaurus/mdx-loader.

This commit/line: 9c92a79#diff-545a046200b390be351720ea1e83af42ec7ed096a4351213383e1550467d8cdcR66

markdownConfig should be an optional type or have a default value assigned, to align with semver.

Reproducible demo

No response

Steps to reproduce

1. Use @docusaurus/mdx-loader in a plugin/app pre 2.2.0
2. Upgrade to 2.2.0
3. Observe: Build breaks with error:

[ERROR] TypeError: Cannot read properties of undefined (reading 'mermaid')
    at Object.mdxLoader (/Users/ben/flagsmith/docs/node_modules/@docusaurus/mdx-loader/lib/loader.js:115:43)
    at LOADER_EXECUTION (/Users/ben/flagsmith/docs/node_modules/loader-runner/lib/LoaderRunner.js:132:14)
    at runSyncOrAsync (/Users/ben/flagsmith/docs/node_modules/loader-runner/lib/LoaderRunner.js:133:4)
    at iterateNormalLoaders (/Users/ben/flagsmith/docs/node_modules/loader-runner/lib/LoaderRunner.js:250:2)
    at /Users/ben/flagsmith/docs/node_modules/loader-runner/lib/LoaderRunner.js:223:4
    at /Users/ben/flagsmith/docs/node_modules/webpack/lib/NormalModule.js:834:15
    at eval (eval at create (/Users/ben/flagsmith/docs/node_modules/tapable/lib/HookCodeFactory.js:33:10), <anonymous>:12:1)
    at processTicksAndRejections (node:internal/process/task_queues:83:21)
    at runNextTicks (node:internal/process/task_queues:65:3)
    at processImmediate (node:internal/timers:437:9)

Can be seen here: cloud-annotations/docusaurus-openapi#222

Expected behavior

Breaking changes should be reserved for major release cycles. markdownConfig should either be optional or have a default value assigned as its value.

Actual behavior

Build process breaks when upgrading to 2.2.0.

Your environment

• Docusaurus version used: v2.2.0
• Environment name and version (e.g. Chrome 89, Node.js 16.4): Chrome Version 107.0.5304.87 (Official Build) (arm64), Node.js v18.11.0
• Operating system and version (e.g. Ubuntu 20.04.2 LTS): Mac OS 13.0

Self-service

• I'd be willing to fix this bug myself.

[Josh-Cena]

We do assume only content plugins use mdx-loader, because it's not a documented public API. Changes to its API surface are not a breaking change. Is it possible for docusaurus-openapi to add markdownConfig: siteConfig.markdown, to its mdx-loader options?

[sean-perkins]

Thanks for the reply @Josh-Cena. Yes, we can default a value for markdownConfig for this plugin to resolve the issue.

While I would expect changes to an undocumented API surface, I would not expect API additions to result in broken builds/types. This seems problematic as an unknown number of plugins could be impacted by this, without much visibility to this change in the CHANGELOG/commits.

This issue seems resolvable at both ends. The plugin can backfill this option to add support for 2.2.0. Docusaurus could either make markdownConfig optional (it appears to only be used for the mermaid feature currently?) or assign an empty object. Would you accept a contribution to make this change to docusaurus and if so, which one would you prefer? Alternatively if you have another suggestion, let me know.

[Josh-Cena]

I would maintain the ground that undocumented APIs are internal and we shouldn't send the message that we care about breaking users of these internal APIs, at the cost of complicating our logic. This is stated in the docs:

For non-theme APIs, any documented API is considered public (and will be stable); any undocumented API is considered internal.

We do normalize the siteConfig.markdown field and apply default values, so as long as you pass this field into markdownConfig, mdx-loader should be happy. It's a relatively easy fix. If it's not easy to work around, I might be okay with changes on our side; but as it stands, I would prefer no changes to our code.

[sean-perkins]

Ok cool. I think we can close out this issue then 👍 I have a viable change to the plugin and others that run into a similar issue can come across this issue as a reference.

Thanks for the quick follow-up!

[Josh-Cena]

Welcome! Thanks for letting us know :)

[slorber]

Added in the Mermaid PR that we recommend third-party plugins using mdx loader to forward the new markdownConfig option:

const loader = {
  loader: require.resolve('@docusaurus/mdx-loader'),
  options: {
    markdownConfig: siteConfig.markdown,
  },
}

More options will be added later, to control the global markdown behavior of your site

[jnystad]

While I understand your reasoning regarding private API changes, it does seem like an easily avoided breaking change (for plugins using mdx-loader directly).

Anyway, could you at least update the documentation for the mdx-loader to indicate this value and that it is required?

It is missing here:
https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-mdx-loader/README.md

[slorber]

handled in #8419

note this retro compatible change will be reverted in v3 (part of #8288) so make sure to upgrade your plugin code. In the future we want the loader to receive the global site markdown config so we do want an error if it's not provided.