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

[WIP] Replace Highlight.js with Prism #4400

Closed
wants to merge 7 commits into from
Closed

Conversation

snide
Copy link
Contributor

@snide snide commented Dec 21, 2020

Summary

Replaces our usage of highlight.js with Prism through https://github.com/react-syntax-highlighter/react-syntax-highlighter

Gives us the ability to add virtualization at some point.

Still has the following issues:

  • Error with props tabs
  • Need to replace remark-highlight in EuiMarkdownEditor
  • Language styling in inline mode of EuiCode

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@thompsongl thompsongl mentioned this pull request Jan 11, 2021
10 tasks
@thompsongl
Copy link
Contributor

This PR came up during today's sync, so I pulled it down to see how much work is left.

Got it updated with upstream before realizing that highlight.js is still used in the PrismAsyncLight component 😳
The "Error with props tabs" task here is actually due to some error with highlight.js parsing, even though EUI no longer uses it directly in this branch.
In react-syntax-highlighter, the base async component is instantiated with highlight.js

I don't have any ideas at the moment, but this casts doubt on being able to use react-syntax-highlighter

cc// @chandlerprall

@snide
Copy link
Contributor Author

snide commented Feb 9, 2021

Yep. I wondered if we would just want to use Prism directly. The primary reason I went to react-syntax-highlighter was because of the promise of a vritualized renderer. I never got that working properly... so we may just want to go with prism directly. At least the styling should be salvagable.

I knew it has hightlight.js as a dep, but thought it was an either/or situation.

@thompsongl
Copy link
Contributor

I knew it has hightlight.js as a dep, but thought it was an either/or situation.

Same. I was surprised when I went digging.

@thompsongl
Copy link
Contributor

https://github.com/FormidableLabs/prism-react-renderer looks pretty interesting, as an alternative

@snide
Copy link
Contributor Author

snide commented Feb 10, 2021

@thompsongl That looks like it'd be a pretty seamless switch and still supports the CSS side with a JS theme that we could use with emotion. Lemme see if I can get you a fresh PR similar to this one. Then I can pass it back. Won't close out virtualization, but we can likely add that on our own with react-window in a later step.

@thompsongl
Copy link
Contributor

thompsongl commented Mar 10, 2021

After talking with @chandlerprall yesterday about the highlight.js timeline, I started looking into this again.
I already have a 1:1 replacement of EuiCode[Block] working using refractor (same prism.js-based engine behind @snide's react-syntax-highlighter implementation), but have some questions.

First some background:

  • refractor works with virtual DOM. This is great for us, but excludes code highlighting on pre-existing HTML. That is, code formatted with extraneous HTML (e.g., I separate lines of code with <br /> but don't expect the <br /> to show in the code block), not code that consists of HTML markup (e.g., <div>Hello</div>).
  • Standard prism.js will strip pre-existing HTML unless you add a plugin to prevent it. Using the standard lib instead of the VDOM lib would still come with caveats and workarounds.
  • Removing highlight.js also requires removing remark-highlight.js. This is no problem; it's very easy to use/build a prism.js/refractor plugin. But consider that all decisions related to prism.js/refractor affect EuiMarkdownEditor and its code plugin as well.

So Do/Did we actually intend to support non-string/non-code children in EuiCode[Block]? highlight.js made this decision irrelevant because it does not strip pre-existing HTML and can parse/ignore it. It would constitute a breaking change if we changed children to only accept string and/or string[]. My opinion is that this is acceptable, but want to get your thoughts.

Another change is that prism.js does not (and will not) support auto-detecting the code language. highlight.js supported auto-detection, and is somewhat the cause of its large size and inefficiency.

cc// @cchaos on this also

@snide
Copy link
Contributor Author

snide commented Mar 10, 2021

To get to the heart of your question. I think it's perfectly acceptable for it to ONLY accept string content.

@thompsongl
Copy link
Contributor

I have just about enough work completed to open a new PR. It might be easier to understand the differences and what breaking changes will be introduced.
Will close this PR when I open a new one and discussion can move over.

@thompsongl
Copy link
Contributor

Closing this PR; moving discussion to #4638

@thompsongl thompsongl closed this Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants