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

Propose: Better support React simple use cases #2537

Closed
joshgoebel opened this issue May 6, 2020 · 17 comments
Closed

Propose: Better support React simple use cases #2537

joshgoebel opened this issue May 6, 2020 · 17 comments
Labels
discuss/propose Proposal for a new feature/direction good first issue Should be easier for first time contributors help welcome Could use help from community parser
Milestone

Comments

@joshgoebel
Copy link
Member

joshgoebel commented May 6, 2020

Creating this thread to discuss if we might provide a simple/small React plugin/component to make using Highlight.js as part of a React application a little easier to get started.

@joshgoebel joshgoebel added parser discuss/propose Proposal for a new feature/direction labels May 6, 2020
@joshgoebel
Copy link
Member Author

First response from the original mixed thread:


For React.js I use the following simple wrapper.
I can't use highlightBlock as it writes directly to DOM node's innerHTML, that is not React way.

import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import hljs from 'highlight.js/lib/core';
import excel from 'highlight.js/lib/languages/excel';

hljs.registerLanguage('excel ', excel);

const SyntaxHighlighter = ({ sourceCode }) => {
    const [parsedCode, setCode] = useState(null);

    useEffect(() => {
        function highlight() {
            let highlighted = hljs.highlight('excel', sourceCode);
            setCode(highlighted.value);
        }
        highlight(sourceCode);
    }, [sourceCode]);

    return (
        <div>
            <pre
                contentEditable={false}
                className="excel"
                dangerouslySetInnerHTML={{ __html: parsedCode }}
            />
        </div>
    );
};

SyntaxHighlighter.propTypes = {
    sourceCode: PropTypes.string,
};

SyntaxHighlighter.defaultProps = {};

export default React.memo(SyntaxHighlighter);

Originally posted by @andrewQwer in #2535 (comment)

@silverwind
Copy link

silverwind commented May 9, 2020

I would not recommend dangerouslySetInnerHTML under any condition. highlightBlock can be used via refs and useEffect.

@andrewQwer
Copy link

highlightBlockmodifies innerHtml, that is not React way of working with DOM.

@silverwind
Copy link

silverwind commented May 9, 2020

I agree it's not ideal if you have to use them but dangerouslySetInnerHTML is even worse as it can introduce security issues when highlighting untrusted code unless hljs can sanitize. Maybe a wrapper module for that outputs a react element for given source code and language would be better.

@joshgoebel
Copy link
Member Author

highlight escapes passed code... the output will only include HTML generated by Highlight.js itself. highlightBlock preserves HTML already inside the block but still properly escapes any code itself that is re-rendered.

I would not recommend dangerouslySetInnerHTML under any condition. highlightBlock can be used via refs and useEffect.

So using code via refs that itself calls innerHTML is seen as better than using dangerouslySetInnerHTML? Aren't they effectively the same?

@joshgoebel
Copy link
Member Author

https://stackoverflow.com/questions/37337289/react-js-set-innerhtml-vs-dangerouslysetinnerhtml

This seems to be arguing in favor of dangerouslySetInnerHTML because it educates React about the block - that it's dynamic and react can ignore it from the shadow DOM, etc.

@joshgoebel
Copy link
Member Author

joshgoebel commented May 9, 2020

In Vue we can just make the highlighted code a computed property and then insert it into the template via variable... passing the code into the component as a prop... surely this type of thing is also possible in React?

The sample above looks a lot like I'd expect that might look but I dunno about all the effect stuff and if there is a simpler way to write it - or if React just requires more boilerplate than Vue.

Highlight.js generates raw HTML... that by it's nature is a [potentially] dangerous thing...

@joshgoebel
Copy link
Member Author

joshgoebel commented May 9, 2020

IE, I think the "simplest" form in Vue might look something like:

export const Component = {
  props: ["language", "code"],
  computed: {
    className( ... ) {},
    highlightedCode() {
      this.highlightedCode = hljs.highlight(...)
    }
  },
  template: `<pre><code :class="className" v-html="highlightedCode"></code></pre>`
};

What would the equivalent be in React?

@silverwind
Copy link

silverwind commented May 10, 2020

I guess it is fine if hljs already escapes dangerous characters in the source. Minimal example could be

function Component({language, code}) {
  const [highlightedCode, setHighlightedCode] = useState(code);

  useEffect(() => {
    setHighlightedCode(hljs.highlight(language, code));
  }, [code]);

  return (
    <pre>
      <code dangerouslySetInnerHTML={{__html: highlightedCode}}/>
    </pre>
  );
};

This will initially render unhighlighted and do another render once highlighting is done. I guess one could block the first render until highlighting is done, but it'll be more complicated.

@joshgoebel
Copy link
Member Author

And could you share what that would look like as pure JS without the JSX?

@joshgoebel
Copy link
Member Author

joshgoebel commented May 11, 2020

I guess it is fine if hljs already escapes dangerous characters in the source.

We currently do > < &... it looks like we should maybe add a few?

&   ------->   &amp;
<   ------->   &lt;
>   ------->   &gt;
"   ------->   &quot;
'   ------->   &#x27;
/   ------->   &#x2F;

Should that cover it? I'm not sure (off the top of my head) how the quotes or / could break out on their own though without the tag characters... I think the quotes are more about raw insertion anywhere (like in the middle of an HTML attribute) as shown here:

https://webmasters.stackexchange.com/questions/12335/should-i-escape-the-apostrophe-character-with-its-html-entity-39

That's not what we do... although now I'm wondering if there is a potential attack vector with a evilly coded grammar via className. Although grammars already run any JS they want freely though, so some sort of attack via className would be the HARD way to do it.

@silverwind
Copy link

Here's a JSX-less working example: https://codesandbox.io/s/intelligent-dubinsky-zkkrj

@joshgoebel joshgoebel modified the milestones: 10.1, 10.2 Jun 11, 2020
@joshgoebel joshgoebel modified the milestones: 10.2, 10.3 Aug 6, 2020
@joshgoebel joshgoebel modified the milestones: 10.3, 11.0 Sep 5, 2020
@joshgoebel joshgoebel added good first issue Should be easier for first time contributors help welcome Could use help from community labels Sep 21, 2020
@joshgoebel
Copy link
Member Author

Pulling in code from external site inline:

import { createElement, useState, useEffect } from "react";
import { render } from "react-dom";
import { highlight } from "highlight.js";
import "highlight.js/styles/default.css";

function Component({ language, code }) {
  const [highlightedCode, setHighlightedCode] = useState(code);

  useEffect(() => {
    setHighlightedCode(highlight(language, code).value);
  }, [language, code]);

  return createElement(
    "pre",
    null,
    createElement("code", {
      dangerouslySetInnerHTML: { __html: highlightedCode }
    })
  );
}

render(
  createElement(Component, {
    language: "javascript",
    code: "const a = 1;"
  }),
  document.getElementById("root")
);

@silverwind Is there no way to do this without pulling in a ton of React requires? We are not a React project and do NOT want to add React to our dependencies... We just need a tiny little "stand-alone" snippet that someone can "register" with React. The build product would be a "stand-alone" JS file that someone just adds to their site. This was trivial with Vue.js. Does React have nothing similar? The goal:

<script src="react.js">
<script src="highlight.js">
<!-- perhaps one line of registration? -->
<!-- now the react component is available and just works -->

Does this not exist in the React world?

@silverwind
Copy link

silverwind commented Oct 8, 2020

You can omit the render and react-dom dependency to only provide the component but stuff imported from react is essential.

Also, one would generally write this in JSX which requires a Babel transform to transpile it to createElement syntax like you've shown, so I don't think it's an actual example one should/could use. I think you should assume a JSX transpiler to be in place, because probably 95%+ of React projects use one.

Just show a dependency installation and JSX example and assume the user has installed a toolchain to build React like create-react-app.

@joshgoebel
Copy link
Member Author

joshgoebel commented Oct 14, 2020

Just show a dependency installation and JSX example and assume the user has installed a toolchain to build React like create-react-app.

Yeah it's starting to sound like perhaps this belongs in documentation somewhere rather than the library... Just having static documentation (ie, a static JS "example") is likely to age poorly over time and not be well maintained. This issue made much more sense when I thought we could perhaps just add a little React "glue" to our library to making using us with React easier for someone "getting started" (ie, like our simple drop-in Vue support). Seems it's much more involved than that.

At this point I think pointing to a well-written/maintained React component that someone in the React community has already written (and could support and answer questions about, etc) is the best route to go. If you know a good one we could mention that'd be great.

Closing this issue.

@alanjohnson
Copy link

In the case of react, is there more to this? I have the exact code as in the above examples, and it works great locally, but when I do "yarn build" it stops with the following error:

Attempted import error: 'highlight.js/lib/core' does not contain a default export (imported as 'hljs').

Well, except i'm using html-react-parser instead of dangerouslySetInnerHTML

@joshgoebel
Copy link
Member Author

joshgoebel commented May 7, 2021

What version is this. If you look at core.js you'll see it has a single default export. Perhaps your build setup is broken or misconfigured?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss/propose Proposal for a new feature/direction good first issue Should be easier for first time contributors help welcome Could use help from community parser
Projects
None yet
Development

No branches or pull requests

4 participants