refactor: handle all admonitions via JSX component #7152

lex111 · 2022-04-11T11:55:54Z

Breaking changes

The previously supported (but undocumented) admonition options are now working differently.

{
        admonitions: {
          customTypes: {
            myType: {
              keyword: `myType`,
              infima: true,
              svg: '<svg /></svg>'
            },
          },
        },
}

Should be refactored as:

{
        admonitions: {
          tag: ':::',
          keywords: [
            'myType' // TYPE ADDED THERE
            'secondary',
            'info',
            'success',
            'danger',
            'note',
            'tip',
            'warning',
            'important',
            'caution',
          ],
        },
}

Also the customization (SVG etc) should be handled with swizzle

Motivation

Attempting to handle all admonitions through the appropriate JSX component. This is not finished version, but POC, there are many issues with typing, but it's works. Since we have a separate JSX component for admonitions, and we even define our own styling for them as admonitions.css, it seems to make sense that the classic theme would be fully responsible for implementing the functionality of admonitions.

What I see as the benefits of this:

Controlling the render of admonitions from one single place
Ensuring admonitions are consistent in terms of markup/design throughout site (docs, blog, pages) regardless of how admonitions are inserted (via MD syntax or as a call to the JSX component)
Swizzling Admonition component will also affect all admonitions, i.e. you can completely change the admonitions using swizzling
Plugins will not duplicate the same dependency
Because admonitions are defined as built-in MDX components, you don't have to import it explicitly as you would do with the JSX component

Technically, new remark plugin have added via configureWebpack hook in a rather non-immutable way, it might be worth creating a new hook to configure remark, something like configureRemark, not sure about that.
Maybe I'm missing some flaws, but it seems like it should be better, if the admonitions will be included from the theme, considering high cohesion between them before.

Fix #7344
Fix #7478

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview: all admonitions should be displayed in the same way as before.

Related PRs

netlify · 2022-04-11T12:04:10Z

✅ [V2]

Name	Link
🔨 Latest commit	`63e9e02`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6299e6426fd902000a46ac50
😎 Deploy Preview	https://deploy-preview-7152--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-04-11T12:04:31Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟢 94	🟢 100	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 83	🟢 100	🟢 100	🟢 100	🟢 90	Report

github-actions · 2022-04-11T12:07:56Z

Size Change: +50 B (0%)

Total Size: 798 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	106 kB	+102 B (0%)
`website/build/assets/js/main.********.js`	600 kB	-52 B (0%)

ℹ️ View Unchanged

Filename	Size
`website/.docusaurus/globalData.json`	52.6 kB
`website/build/index.html`	38.9 kB

_{compressed-size-action}

Josh-Cena

Thanks for bringing this up!

I'm very much in favor of this, and I think everyone agrees this is what we would eventually do (see #6246). However, Sebastien and I talked about this on Discord some time in the past (IIRC), and I think the consensus was that we would only fork dependencies after we've had the @Docusaurus organization properly set up and ready to be populated with forks. I think we are in a pretty awkward situation right now, because we have neither the architecture nor sufficient incentive to make this change happen.

packages/docusaurus-theme-classic/src/theme-classic.d.ts

Josh-Cena · 2022-04-11T12:45:51Z

packages/docusaurus-theme-classic/src/index.ts

+      (config.module?.rules as webpack.RuleSetRule[])?.forEach((rule) => {
+        if (Array.isArray(rule.use)) {
+          // eslint-disable-next-line @typescript-eslint/no-explicit-any
+          rule?.use.forEach((useItem: any) => {
+            if (useItem.loader!.includes('docusaurus-mdx-loader')) {
+              useItem?.options.remarkPlugins.push(admonitions);
+            }
+          });
+        }
+      });


This looks kind of hacky. In the future we can use #6370 to handle this (this is already marked as one of the use-cases of configureMarkdownLoader).

Also, because the fallback MDX loader in the core is pushed after all plugins, I think this would make admonitions unavailable to documents using the fallback loader? Maybe we need to change that order?

It was the only way to push remark plugin into the all MDX loaders. Are admonitions in the fallback loader really necessary?

Yeah, it was added in core specifically because someone brought it up: #5333 it's mostly for partials not in the docs directory. It can be trivially fixed though, we just need to push the synthetic plugins before the others:

docusaurus/packages/docusaurus/src/server/plugins/index.ts

Lines 38 to 44 in 0abf4d7

// 1. Plugin Lifecycle - Initialization/Constructor.

const plugins: InitializedPlugin[] = await initPlugins(context);

plugins.push(

createBootstrapPlugin(context),

createMDXFallbackPlugin(context),

);

lex111 · 2022-04-11T15:13:18Z

I see, but on the other hand it is not that big dependency and not that significant a change after all, so I would suppose that at this moment we could adopt this solution after fix some issues with the code (in particular with types).

Josh-Cena · 2022-04-11T15:14:08Z

Yeah, I will take a look at the types tomorrow, see what I can help to fix

Josh-Cena · 2022-04-12T11:53:30Z

This is hard. Because the MDX fallback plugin would also search for all existing MDX loaders and only include those files that are not yet included, putting it before the content plugins will result in duplicate loaders and then a dead cycle.

What about we move this remark plugin to the MDX loader? Since it's generic enough (only requires the existence of some MDX mapping from <admonition> to a real component), it doesn't necessarily have to be provided by theme-classic. Our previous architecture where the core pushes an admonition plugin has been horrible enough, it can't be any worse than that.

Josh-Cena · 2022-04-12T12:07:58Z

packages/docusaurus-theme-classic/src/remark/admonitions/index.ts

+    const element = {
+      type: 'admonitionHTML',
+      data: {
+        hName: 'admonition',


I think this is a pretty good step. This also means users can swizzle MDXComponents and re-map admonition to some other component, not necessarily @theme/Admonition.

👍 totally in favor of this

Josh-Cena · 2022-04-12T14:16:31Z

packages/docusaurus-theme-classic/src/index.ts

+            ) {
+              useItem.options ??= {};
+              (useItem.options as MDXOptions).remarkPlugins ??= [];
+              (useItem.options as MDXOptions).remarkPlugins.push(admonitions);


We need to support custom admonitions as we previously did. We have users heavily using them.

agree 👍

can you show example sites using custom admonitions and the configs used?

Site: https://sonceri.art/docs/intro
Source: https://github.com/lorikrell/gamerchic/blob/ec6e1595a51b7e65538262dd9308fb7517e42f9f/docusaurus.config.js#L359-L402

@Josh-Cena I think we should also study how changes in this PR will look like after MDX 2 and https://github.com/remarkjs/remark-directive

I don't think the current API for admonition customization will stay as is, and instead we'll do customizations through swizzle?

So maybe we can already do the breaking change today if it's already planned anyway 🤷‍♂️

lex111 · 2022-04-12T18:50:17Z

Thanks for the fixes!

What about we move this remark plugin to the MDX loader? Since it's generic enough (only requires the existence of some MDX mapping from to a real component), it doesn't necessarily have to be provided by theme-classic. Our previous architecture where the core pushes an admonition plugin has been horrible enough, it can't be any worse than that.

Just the original idea was to keep all the related things together, i.e. the remark plugin, the Admonition component, its styles, altogether with the definition in MDX components. So putting only the remark plugin in the mdx-loader would contrary to that idea. Maybe we should create new a simplified and undocumented lifecycle hook to deal with this for now?

Josh-Cena · 2022-04-12T23:31:03Z

You mean, configureMDXLoader? TBF, I don't even have a clear plan how to do that yet... Let's see what @slorber thinks

lex111 · 2022-04-13T07:05:04Z

Yes, something similar, but so far in a simplified form, enough to add that new remark plugin.

slorber

Thanks 👍

In favor of doing, clearly better for customizations to use our theme component consistently 👍

Will have to check the copyright thing

slorber · 2022-04-14T10:09:16Z

packages/docusaurus-theme-classic/src/remark/admonitions/index.ts

@@ -0,0 +1,136 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.


This is basically a modified copy of MIT-based code from https://github.com/elviswolcott/remark-admonitions/blob/master/lib/index.js on which you put Facebook copyright and not mention previous copyright

Technically I'm fine "owning" this code in the Docusaurus codebase, but I'm not sure this can be done this way legally, will have to check

Also we can probably publish this as a separate standalone package as it could be useful in other contexts.

We may eventually create a more generic tool that transforms custom syntax to JSX/MDX?

For example, we could also create a new custom syntax to handle tabs, similarly to this:

https://retype.com/components/tab/#multiple-tabs

(not so sure, maybe after we upgrade to mdx 2 we'll just need https://github.com/remarkjs/remark-directive)

Looks like we can likely drop a LICENSE file in a subfolder to preserve the original license

Example: https://github.com/zpao/qrcode.react/tree/main/src/third-party/qrcodegen

Is all code in this subfolder coming from the original repo? Otherwise we can split between new files + copied files

@slorber It's not a verbatim copy (and in fact significantly refactored, if not a total rewrite), so we need to have the licenses of both.

Yes I know, trying to figure this out. The above copy example has also been modified btw.

slorber · 2022-04-14T10:09:48Z

packages/docusaurus-theme-classic/src/remark/admonitions/index.ts

+    const element = {
+      type: 'admonitionHTML',
+      data: {
+        hName: 'admonition',


👍 totally in favor of this

packages/docusaurus-theme-classic/src/theme/MDXComponents/index.tsx

slorber · 2022-04-14T10:23:16Z

packages/docusaurus-theme-classic/src/index.ts

+            ) {
+              useItem.options ??= {};
+              (useItem.options as MDXOptions).remarkPlugins ??= [];
+              (useItem.options as MDXOptions).remarkPlugins.push(admonitions);


agree 👍

can you show example sites using custom admonitions and the configs used?

slorber · 2022-04-14T10:33:38Z

packages/docusaurus-plugin-content-docs/src/options.ts

@@ -168,11 +167,5 @@ export function validateOptions({

  const normalizedOptions = validate(OptionsSchema, options) as PluginOptions;

-  if (normalizedOptions.admonitions) {


this is removed, but admonitions remains in schema + default value

We want to still allow custom admonitions? Are custom admonitions configured globally or per plugin?

Do we want to do a breaking change and add a custom error message, or try to keep retro-compatibility?

I think we should keep this here for the time being, but when we eventually have a centralized markdown.remarkPlugins config (#5999) I'd like to move it there. Note that I also want to have admonitions as part of the default MDX loader preset, so it can be configured as part of the docusaurus-remark-preset.

@Josh-Cena I agree we should keep the options there, at least for now

The problem is that the provided options do not make sense anymore

https://github.com/lorikrell/gamerchic/blob/ec6e1595a51b7e65538262dd9308fb7517e42f9f/docusaurus.config.js#L359

{ admonitions: { customTypes: { homebrew: { keyword: `homebrew`, infima: true, svg: '<svg /></svg>' }, gamerchic: { keyword: `gamerchic`, infima: true, svg: '<svg /></svg>' }, }, }, }

Now the options should look like:

{ admonitions: { customKeywords: ["homebrew","gamerchic"] }, }

And custom styling/svg should be applied by swizzling the admonition component.

@lex111 it's worth supporting that to ensure that the above site config have a way to upgrade Docusaurus without losing its custom admonitions

We also need to add option schema changes so that it prints a clear error message when user is trying to use the "old" customizations format

slorber · 2022-04-14T10:40:27Z

packages/docusaurus-theme-classic/src/index.ts

+        ),
+      );
+
+      config.module?.rules?.forEach((rule) => {


🤪 not a fan of this

Maybe we could just add an mdx-loader "admonitions" option on which we'd just forward the plugin options (if we keep them?)

I know it's not ideal to add everything to mdx-loader but it's probably a good-enough temporary solution until we figure out something better

As I explained above, the idea was for the theme to provide admonitions functionality, so the remark plugin should add from the theme. That's why I suggested to implement a simplified lifecycle hook to solve only this issue for now.

you can make the loader provide an optional "admonitions" feature that the theme would enable

it's less flexible but would definitively work without needing any extra lifecycle, giving us time to figure out a good API for that lifecycle. If we can avoid introducing temporary experimental public apis that's better

you can make the loader provide an optional "admonitions" feature that the theme would enable

Do you mean move the admonitions plugin to the mdx-loader or something else?

Yes, for now, you can move the admonition plugin to mdx loader and pass an admonition: {customKeywords: []} option. We'll likely move it outside later but for now it's the simplest solution.

# Conflicts: # packages/docusaurus-plugin-content-blog/package.json # packages/docusaurus-plugin-content-blog/src/index.ts # packages/docusaurus-plugin-content-docs/package.json # packages/docusaurus-plugin-content-pages/package.json

lex111 · 2022-05-06T14:55:05Z

Thanks for finalizing this PR, I just migrated the styles to CSS modules and also finally fixed the longstanding a11y problem related with headings.

Josh-Cena · 2022-05-06T14:57:41Z

Yes, that's great! We can merge this as soon as we get confirmation about the licensing.

Josh-Cena · 2022-05-22T04:56:06Z

GitHub supports the new admonition syntax:

> **Note**
> a note
> A note...

Note
a note
A note...

Once we merge this, this can be easily implemented in user-land.

lebalz · 2022-05-22T10:51:14Z

packages/docusaurus-mdx-loader/src/remark/admonitions/index.ts

+
+    let newValue = value;
+    // consume lines until a closing tag
+    let idx = newValue.indexOf(NEWLINE);


Not sure if this can be done in this PR or in a followup. But i would suggest to support nested Admonitions aswell, s.t. something like

:::info Weather On nice days, you can enjoy swimming in the lake. :::danger Storms Take care of summerstorms... ::: :::

would work as expected.

To do this, we would need to keep track of the nesting level. I already did something similar for elviswolcott/remark-admonitions#41 here: https://github.com/elviswolcott/remark-admonitions/pull/42/files#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1R170

Would be nice to see this supported here aswell.

Thanks for that info @lebalz Let's do that as a follow-up because this one is supposed to be a simple port.

Josh-Cena

There's a regression: https://deploy-preview-7152--docusaurus-2.netlify.app/docs/api/misc/@docusaurus/logger/

Markdown inside admonition title is no longer interpolated.

I don't know it's serious enough to block this PR, but want to bring that to people's attention. I deliberately included that text in the documentation because we do have users relying on it. See #7173

Josh-Cena · 2022-05-25T09:28:14Z

Another idea: this is a good time to make a breaking change. We should not have individual admonition options for each plugin, but create a global siteConfig.markdown.admonitions config. I don't think anyone would want to turn it off for some plugins anyway—WDYT?

# Conflicts: # packages/docusaurus-plugin-content-pages/src/index.ts

slorber · 2022-06-02T17:29:03Z

Markdown inside admonition title is no longer interpolated.

Damn, really want to merge this one but couldn't find the solution yet 😅

slorber · 2022-06-03T09:31:02Z

After discussing with @wooorm it appears to be quite complex to support an MDX title like

<admonition title={<>my <code>title</code></>}>
  <p>body</p>
</admonition>

This will be easier in MDX v2.

In the meantime, I implemented a temporary workaround where everything goes inside children and the component parses it to render it as a title.

<admonition>
  <mdxAdmonitionTitle>my <code>title</code><mdxAdmonitionTitle/>
  <p>body</p>
</admonition>

function extractMDXAdmonitionTitle(children: ReactNode): {
  mdxAdmonitionTitle: ReactNode | undefined;
  rest: ReactNode;
} {
  const items = React.Children.toArray(children);
  const mdxAdmonitionTitle = items.find(
    (item) =>
      React.isValidElement(item) &&
      (item.props as {mdxType: string} | null)?.mdxType ===
        'mdxAdmonitionTitle',
  );
  const rest = <>{items.filter((item) => item !== mdxAdmonitionTitle)}</>;
  return {
    mdxAdmonitionTitle,
    rest,
  };
}

This is not ideal but good enough to support this edge case and not block this PR

Tests:

https://deploy-preview-7152--docusaurus-2.netlify.app/docs/api/misc/@docusaurus/logger/

https://deploy-preview-7152--docusaurus-2.netlify.app/tests/pages/markdownPageTests#admonitions

slorber · 2022-06-03T09:35:29Z

Another idea: this is a good time to make a breaking change. We should not have individual admonition options for each plugin, but create a global siteConfig.markdown.admonitions config. I don't think anyone would want to turn it off for some plugins anyway—WDYT?

Will add that in a separate PR

Josh-Cena · 2022-06-03T09:36:52Z

Looks good enough to me👍 This is what I figured out as well.

Josh-Cena · 2022-06-03T09:55:20Z

The PR look good at a glance now, will try more stuff locally when I get to my keyboard

Josh-Cena

We have some pending followup work to do, but this is good to go!

refactor: handle all admonitions via JSX component

d02485a

lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Apr 11, 2022

lex111 requested review from slorber and Josh-Cena as code owners April 11, 2022 11:55

facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 11, 2022

Josh-Cena reviewed Apr 11, 2022

View reviewed changes

fixes

3b74726

Josh-Cena reviewed Apr 12, 2022

View reviewed changes

slorber reviewed Apr 14, 2022

View reviewed changes

slorber marked this pull request as draft April 14, 2022 10:43

slorber mentioned this pull request Apr 20, 2022

"Docs plugin enabled" detection no longer does anything #7207

Open

lex111 mentioned this pull request May 4, 2022

chore: upgrade Infima to alpha.39 #7306

Merged

3 tasks

slorber added 8 commits May 5, 2022 16:48

Merge branch 'main' into lex111/themed-admonitions

d2f18d8

# Conflicts: # packages/docusaurus-plugin-content-blog/package.json # packages/docusaurus-plugin-content-blog/src/index.ts # packages/docusaurus-plugin-content-docs/package.json # packages/docusaurus-plugin-content-pages/package.json

ability to customize admonition keywords and tag

d3a9711

move remark admonitions to mdx-loader

4b8121f

upgrade AdmonitionsSchema

3bbdbab

upgrade AdmonitionsSchema

00a7e74

upgrade AdmonitionsSchema

f6fb1b0

wire admonition option to all content plugins

1f3fff4

fix admonitions validation schema

25ae510

Merge branch 'main' into lex111/themed-admonitions

0e9e357

Merge branch 'main' into lex111/themed-admonitions

cc0719f

lebalz reviewed May 22, 2022

View reviewed changes

This was referenced May 23, 2022

GFM like admonitions/callouts #7471

Open

Can't swizzle Admonition #7478

Closed

Josh-Cena added 3 commits May 24, 2022 17:27

Merge branch 'main' into lex111/themed-admonitions

0e4663c

Merge branch 'main' into lex111/themed-admonitions

233ca61

Merge branch 'main' into lex111/themed-admonitions

a18f60b

Josh-Cena requested changes May 25, 2022

View reviewed changes

Merge branch 'main' into lex111/themed-admonitions

2e82db9

# Conflicts: # packages/docusaurus-plugin-content-pages/src/index.ts

Implement workaround to support interpolation in MDX admonition titles

01f669c

add link to relevant issue comment #7152 (comment)

287e2f0

Little refactors

63e9e02

Josh-Cena approved these changes Jun 3, 2022

View reviewed changes

slorber merged commit 5746c58 into main Jun 3, 2022

slorber deleted the lex111/themed-admonitions branch June 3, 2022 12:26

slorber mentioned this pull request Jun 3, 2022

Translate admonition keyword elviswolcott/remark-admonitions#35

Closed

slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Jul 8, 2022

swiatekm-sumo mentioned this pull request Jul 13, 2022

Upgrade docusaurus to 2.0.0-beta.22 SumoLogic/sumologic-documentation#88

Merged

4 tasks

jwitz mentioned this pull request Jul 26, 2022

Create a custom admonition for CLI callouts astronomer/docs#958

Closed

Djunnni mentioned this pull request Oct 25, 2022

Admonitions with backticks elicit <mdxAdmonitionTitle /> warnings #8233

Closed

7 tasks

	// 1. Plugin Lifecycle - Initialization/Constructor.
	const plugins: InitializedPlugin[] = await initPlugins(context);

	plugins.push(
	createBootstrapPlugin(context),
	createMDXFallbackPlugin(context),
	);

		@@ -0,0 +1,136 @@
		/**
		* Copyright (c) Facebook, Inc. and its affiliates.

		@@ -168,11 +167,5 @@ export function validateOptions({

		const normalizedOptions = validate(OptionsSchema, options) as PluginOptions;

		if (normalizedOptions.admonitions) {

refactor: handle all admonitions via JSX component #7152

refactor: handle all admonitions via JSX component #7152

Conversation

lex111 commented Apr 11, 2022 • edited by slorber

Breaking changes

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

netlify bot commented Apr 11, 2022 • edited

✅ [V2]

github-actions bot commented Apr 11, 2022 • edited

⚡️ Lighthouse report for the deploy preview of this PR

github-actions bot commented Apr 11, 2022 • edited

Josh-Cena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena Apr 11, 2022 • edited

Choose a reason for hiding this comment

lex111 commented Apr 11, 2022

Josh-Cena commented Apr 11, 2022

Josh-Cena commented Apr 12, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lex111 commented Apr 12, 2022

Josh-Cena commented Apr 12, 2022

lex111 commented Apr 13, 2022

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber Apr 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lex111 commented May 6, 2022

Josh-Cena commented May 6, 2022

Josh-Cena commented May 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josh-Cena left a comment

Choose a reason for hiding this comment

Josh-Cena commented May 25, 2022

slorber commented Jun 2, 2022

slorber commented Jun 3, 2022 • edited

slorber commented Jun 3, 2022

Josh-Cena commented Jun 3, 2022

Josh-Cena commented Jun 3, 2022 • edited

Josh-Cena left a comment

Choose a reason for hiding this comment

lex111 commented Apr 11, 2022 •

edited by slorber

netlify bot commented Apr 11, 2022 •

edited

github-actions bot commented Apr 11, 2022 •

edited

github-actions bot commented Apr 11, 2022 •

edited

Josh-Cena Apr 11, 2022 •

edited

Josh-Cena commented Apr 12, 2022 •

edited

slorber Apr 14, 2022 •

edited

Josh-Cena Apr 20, 2022 •

edited

slorber Apr 20, 2022 •

edited

slorber commented Jun 3, 2022 •

edited

Josh-Cena commented Jun 3, 2022 •

edited