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

feat(v2): Allow swizzling prism-include-languages in theme-classic #2841

Merged
merged 3 commits into from
May 31, 2020
Merged

feat(v2): Allow swizzling prism-include-languages in theme-classic #2841

merged 3 commits into from
May 31, 2020

Conversation

SamChou19815
Copy link
Contributor

Motivation

Right now it's hard to include custom languages syntax highlighting support. It's almost impossible to do it in a simple config due to context replacement issue. Right now there are two alternatives that both have some issues:

  1. Swizzle CodeBlock component. Issue: A local CodeBlock might eventually diverge from the CodeBlock in theme-classic.
  2. Write a plugin. This is the approach I am currently using. See (1) and (2). However, whether the Prism object used by theme-classic will be correctly patched is completely at the mercy of package manager's hoisting behavior:

Suppose you do this in your plugin

import Prism from 'prism-react-renderer/prism';

If you declare prism-react-renderer as a dependency of your plugin, then you might end up patching a prism-react-renderer that is not used by theme-classic, since you might get a node_modules layout like:

- your-plugin
  - node_modules
    - prism-react-renderer
- @docusaurus/theme-classic
  - node_modules
    - prism-react-renderer

If you declare prism-react-renderer as a peer dependency or doesn't declare as a dependency, then you are still relying on package manager's hoisting behavior that produces this node_modules layout:

- node_modules
  - your-plugin
  - prism-react-renderer
  - @docusaurus/theme-classic

However, it's completely legal for a package manager to produce this:

- node_modules
  - your-plugin
  - @docusaurus/theme-classic
    - node_modules
      - prism-react-renderer

Then your plugin cannot find prism-react-renderer any more.

The root of the issue is that we need a way that is guaranteed to use prism-react-renderer from @docusaurus/theme-classic, while still being able to be customized easily in userland. This leaves us with only one alternative that doesn't require significant refactoring: Swizzling.

This diff moves the main implementation of prism-include-languages into src/theme folder so that it can be swizzled. The API of @theme/prism-include-languages is likely to be very stable so swizzling it wouldn't produce a huge risk of divergence.

Questions for maintainers

  1. I noticed in the discussion that docusaurus might try to move away from swizzling. So I wonder whether this diff is a good idea.
  2. If this is approved, should we still keep the additionalLanguages config?

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  1. Swizzle prism-include-languages by yarn workspace docusaurus-2-website swizzle @docusaurus/theme-classic prism-include-languages
  2. Edit docusaurus.config.js to include swift as an additional language:
    prism: {
      theme: require('prism-react-renderer/themes/github'),
      darkTheme: require('prism-react-renderer/themes/dracula'),
+     additionalLanguages: ['swift']
    },
  1. Add some random swift code to a docs page, like
protocol Foo {}
  1. yarn start, and see the code is correctly syntax highlighted.

Screen Shot 2020-05-29 at 11 55 55

5. Edit the swizzled `prism-include-languages` to make it an empty function.
  1. Restart, now it's no longer syntax highlighted, which shows that the swizzled component has been correctly picked up.

Screen Shot 2020-05-29 at 11 56 20

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 29, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented May 29, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 20ccbb9

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

@SamChou19815 SamChou19815 marked this pull request as ready for review May 29, 2020 16:43
@SamChou19815 SamChou19815 requested a review from lex111 as a code owner May 29, 2020 16:43
@lex111
Copy link
Contributor

lex111 commented May 29, 2020

It’s definitely certain that we won’t be able to completely refuse from swizzling, this is actually not a bad mechanism for change built-in components.

We will also leave additionalLanguages option, because otherwise our users will need to have swizzled prism-include-languages to add more languages -- this is an unnecessary step and just unpleasant action for them.

Therefore, I think we need to update the docs for this new use case - when people want to add custom language. However in most cases this is not needed by anyone, since very few people develop their own programming languages, since this feature is intended only for them, right?

@SamChou19815 SamChou19815 requested a review from yangshun as a code owner May 29, 2020 17:22
@SamChou19815
Copy link
Contributor Author

Therefore, I think we need to update the docs for this new use case - when people want to add custom language. However in most cases this is not needed by anyone, since very few people develop their own programming languages, since this feature is intended only for them, right?

That's true. I've added the docs.

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Some improvements.

website/docs/markdown-features.mdx Outdated Show resolved Hide resolved
website/docs/markdown-features.mdx Outdated Show resolved Hide resolved
website/docs/markdown-features.mdx Outdated Show resolved Hide resolved
website/docs/markdown-features.mdx Show resolved Hide resolved
@yangshun yangshun merged commit 93b35af into facebook:master May 31, 2020
@SamChou19815 SamChou19815 deleted the allow-swizzle-prism-include-languages branch May 31, 2020 07:05
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Jun 17, 2020
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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants