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): add support specify new languages for Prism #2250

Merged
merged 6 commits into from
Mar 8, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Jan 29, 2020

Motivation

Resolve #1879 and close #2356 as our users have an interest in this feature, I think we need to add this.

Related issues on Canny:

Cons of the current implementation:

  • Pollution of global space, I do not know how to avoid this, given the use of Prism React Renderer :(

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  1. Add themeConfig.prism.additionalLanguages array with any languages missing in Prism React Renderer (rust eg)
  2. Add corresponding code block, eg for rust:
```rust
fn main() {
    // Statements here are executed when the compiled binary is called

    // Print text to the console
    println!("Hello World!");
}
  1. Make sure that it syntax highlighting for the added code block works correctly.

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

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Jan 29, 2020
@lex111 lex111 requested a review from yangshun January 29, 2020 03:56
@lex111 lex111 requested a review from wgao19 as a code owner January 29, 2020 03:56
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 29, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jan 29, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 6f99c05

https://deploy-preview-2250--docusaurus-2.netlify.com

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Since this is just about importing more modules, can we add those lines to getClientModules() of packages/docusaurus-theme-classic/src/index.js? You have access to themeConfig there. That way we don't have to add any global variables at all.

I think checking whether the language exists on Prism.language is optional and shouldn't be a blocker to use this approach. The checking should be possible in packages/docusaurus-theme-classic/src/index.js as well.

@lex111
Copy link
Contributor Author

lex111 commented Feb 8, 2020

That way we don't have to add any global variables at all.

This will not work, we need to change the global Prism object. We already have a “ready-made” object from prism-react-renderer dep and we need to “modify” it. If we want a better solution, then we need to fork prism-react-renderer, I guess...

I think checking whether the language exists on Prism.language is optional

This check is necessary so as not to included (require) languages that have already been loaded (required). If the language is not supported by Prism, an exception will be thrown if you had this in mind.

@yangshun
Copy link
Contributor

yangshun commented Feb 8, 2020

This will not work, we need to change the global Prism object.

How are we modifying it in the code? Or is requiring the language module modifying it within?

@lex111
Copy link
Contributor Author

lex111 commented Feb 8, 2020

No, I mean that we need an existing Prism object, that is, we can not do support any language without this global variable from prism-react-renderer.

@yangshun
Copy link
Contributor

yangshun commented Feb 8, 2020

Can you add another client module that does the exposing of the Prism object?

@lex111
Copy link
Contributor Author

lex111 commented Feb 8, 2020

But how? Do you want to import Prism globally before loading React? Is this possible in the current implementation?

UPD: Even if this is done, will it really work? https://github.com/FormidableLabs/prism-react-renderer/blob/master/src/defaultProps.js#L10
We use the Highlight component, which gets its own Prism object. It is not clear to me how to exposing this object from the context of client module?

@lex111
Copy link
Contributor Author

lex111 commented Feb 20, 2020

@yangshun please could you investigate if there is another good solution here so I can close this PR?

@yangshun
Copy link
Contributor

I'll take a stab at this over the weekend (or the next :S)

@lex111
Copy link
Contributor Author

lex111 commented Feb 20, 2020

That would be great, although I have no idea how to do it differently, unless to fork prism-react-renderer lib and somehow modify the Prism object 🤷‍♂️

@ark120202
Copy link

Here's is a small plugin that have worked for me:

/** @returns {import('@docusaurus/types').Plugin<any>} */
module.exports = ({
  siteConfig: { themeConfig: { prism: { additionalLanguages = [] } = {} } = {} },
}) => ({
  getClientModules: () => [
    require.resolve('./prism-expose'),
    ...additionalLanguages.map(lang => require.resolve(`prismjs/components/prism-${lang}`)),
    require.resolve('./prism-cleanup'),
  ],
});
// prism-expose.js
import Prism from 'prism-react-renderer/prism';

window.Prism = Prism;
// prism-cleanup.js
window.Prism = undefined;

@lex111
Copy link
Contributor Author

lex111 commented Feb 23, 2020

@ark120202 thank you 👍 , this approach apparently meant @yangshun, but I misunderstood him, and was looking for a solution in a completely different direction. I can rework my PR in accordance with this solution.

@lex111
Copy link
Contributor Author

lex111 commented Feb 26, 2020

@yangshun re-check it out please.

window.Prism = Prism;

additionalLanguages.forEach(lang => {
require(`prismjs/components/prism-${lang}`); // eslint-disable-line

Choose a reason for hiding this comment

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

I might be wrong, but if there's no some context replacement I'm missing, all files matching this query would be included in a bundle

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah agreed, I think so too! One alternative is to generate a file in one of the plugin steps that generates the list of require() statements containing just the languages needed and have this file import it.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

LGTM, my main concern is the dynamic requires.

website/docs/theme-classic.md Outdated Show resolved Hide resolved
website/docs/theme-classic.md Outdated Show resolved Hide resolved
@lex111
Copy link
Contributor Author

lex111 commented Feb 29, 2020

@yangshun I fixed all bugs, please review.

liushooter added a commit to liushooter/docusaurus-theme-classic-new that referenced this pull request Mar 4, 2020
@@ -17,7 +17,8 @@
"prism-react-renderer": "^1.0.2",
"react-router-dom": "^5.1.2",
"react-toggle": "^4.1.1",
"remark-admonitions": "^1.1.0"
"remark-admonitions": "^1.1.0",
"webpack": "^4.41.2"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be part of devDependencies right?


return {
plugins: [
new ContextReplacementPlugin(
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments on what it does and why it is necessary would be nice.

@yangshun yangshun merged commit 5226767 into facebook:master Mar 8, 2020
@yangshun yangshun deleted the prism-extra-langs branch March 8, 2020 14:13
@bravo-kernel
Copy link
Contributor

@lex111 wow, I think you have just solved one of the most important feature requests/blockers. Amazing job ❤️

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.

Why not support ruby code highlighting? Configurable syntax highlighting languages
6 participants