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

refactor: handle all admonitions via JSX component #7152

Merged
merged 27 commits into from
Jun 3, 2022

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Apr 11, 2022

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

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Apr 11, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 11, 2022
@netlify
Copy link

netlify bot commented Apr 11, 2022

[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...

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
Copy link

github-actions bot commented Apr 11, 2022

⚡️ 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
Copy link

github-actions bot commented Apr 11, 2022

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

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines 171 to 180
(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);
}
});
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@Josh-Cena Josh-Cena Apr 11, 2022

Choose a reason for hiding this comment

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

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:

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

@lex111
Copy link
Contributor Author

lex111 commented Apr 11, 2022

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
Copy link
Collaborator

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

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Apr 12, 2022

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.

const element = {
type: 'admonitionHTML',
data: {
hName: 'admonition',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 totally in favor of this

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree 👍

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

lex111 commented Apr 12, 2022

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
Copy link
Collaborator

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

@lex111
Copy link
Contributor Author

lex111 commented Apr 13, 2022

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

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks 👍

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

Will have to check the copyright thing

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@slorber slorber Apr 14, 2022

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@Josh-Cena Josh-Cena Apr 20, 2022

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

const element = {
type: 'admonitionHTML',
data: {
hName: 'admonition',
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 totally in favor of this

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

Choose a reason for hiding this comment

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

agree 👍

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

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

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

if (normalizedOptions.admonitions) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@slorber slorber Apr 20, 2022

Choose a reason for hiding this comment

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

@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

),
);

config.module?.rules?.forEach((rule) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤪 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

lex111 commented May 6, 2022

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
Copy link
Collaborator

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

@Josh-Cena
Copy link
Collaborator

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.


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

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

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

image

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
Copy link
Collaborator

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
Copy link
Collaborator

slorber commented Jun 2, 2022

Markdown inside admonition title is no longer interpolated.

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

@slorber
Copy link
Collaborator

slorber commented Jun 3, 2022

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

CleanShot 2022-06-03 at 11 32 21@2x

CleanShot 2022-06-03 at 11 32 33@2x

@slorber
Copy link
Collaborator

slorber commented Jun 3, 2022

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
Copy link
Collaborator

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

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Jun 3, 2022

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

Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

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

@slorber slorber merged commit 5746c58 into main Jun 3, 2022
@slorber slorber deleted the lex111/themed-admonitions branch June 3, 2022 12:26
@slorber slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Jul 8, 2022
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: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't swizzle Admonition Admonitions make the Heading hierarchy inconsistent
5 participants