Syntax highlighting for instructions markdown!

[inlinestyle]

Notes:

• remark-react-lowlight emits a React.PropTypes deprecation warning. I've sent them a pull request here: Fix React deprecation warning - use 'prop-types' package inlinestyle/remark-react-lowlight#6
• I've selected the "github" color theme, but there are a bunch of available themes. The "default" theme didn't have great colors for html tags IMHO.
• An unexpected hiccup appeared when I wondered what would happen if I supplied a code block with a language outside the configuration (e.g. "python"). Turns out it crashed the entire app! I ended up adding a new InstructionsErrorBoundary component with a loooong comment. The app now gracefully degrades to non-highlit code when an unconfigured language is supplied. Unfortunately I can't think of a way to make this apply "partially" (only to the offending code block) without parsing the markdown by hand.
• I used state to store whether our syntax highlighting has errored out; I can move it to redux if state is that undesirable.

Popcode with highlit html/css/js: http://localhost:3000/?gist=911a82a17a280545858d2d8ecc557ef3

Popcode with no highlighting due to a python section: http://localhost:3000/?gist=b2b6a8fa00f160e00ccf9ca0c1895994

[inlinestyle]

UPDATE: The deprecation warning fix in remark-react-lowlight has been merged, and this has been updated to include it.

[outoftime]

@inlinestyle so I’m definitely not in love with the idea of using state—it does make sense in this case though. Perhaps this is irrational but I’m generally much more OK with bringing in external React addon libraries that use state without me having to see it in the code… would https://github.com/bvaughn/react-error-boundary be reasonable to try? It does support the React 15 nonsense, although from looking at usage I can see how it might be kind of a pain to make it do exactly what we need here.

Other high-level feedback is that <InstructionsErrorBoundary> seems to be carrying a lot of water here—lots of knowledge in here that is really core to <Instructions>. Can we make it more tightly focused on just the error boundary responsibility?

LMK your thoughts on the above—definitely open to counterarguments : )

[inlinestyle]

@outoftime I agree that there's a lot in <InstructionsErrorBoundary>; my first inclination is actually to move it to a util file so we could have something like

import markdown from '../util/markdown';
...
export default class Instructions extends React.Component {
...
  render() {
    const {instructions, useHighlighting} = this.props;
    return markdown.toReact(instructions, useHighlighting);
  }
...
}

[inlinestyle]

@outoftime updated with the move to a util file, and lazy loading of the remark renderer.

[outoftime]

@inlinestyle left some thoughts… feels v. close though!

[outoftime]

I’m generally much more into function/method/class-level comments than inline ones… in this case just a matter of moving the comment block a couple lines north, you down?

[inlinestyle]

I'm assuming you mean put it immediately above unstable_handleError, instead of immediately below? To be honest I interpret both as "method-level" but it's no problem to move it.

[outoftime]

Yep, I guess just from the way jsdoc and equivalent systems do it, putting it above the method header feels a bit more natural

[outoftime]

I would argue that for this one, we want to disable the rule at the module level—it makes more sense to me to say “this is a component that uses state” rather than “this is a component that doesn’t use state except for this one place where it does”

[outoftime]

I’m generally not a fan of export defaulting an object of functions—seems like named exports are a more idiomatic way to accomplish the same thing. How do you think about it?

[inlinestyle]

This is probably a holdover from other languages, but I like being able to use the module name to qualify a function... if I want to be able to write markdown.toReact I can use import * as markdown from ... (doesn't need to be a default) or import markdown from ... (needs to be a default).

I'd be fine switching to the import * as markdown from ... version, or abandoning the module qualification thing and changing the declaration to export function markdownToReact but those seem a little less natural. Up to you.

[outoftime]

mm, I definitely feel you and intuitively I too like having the qualifier. Ultimately I don’t have a specific preference to stake out but a couple of observations:

• It seems like import * as markdown is a strict improvement, since you’re still getting the same interface in the importing module, and the exporting module structure is more idiomatic
• It doesn’t seem very common to do e.g. import {toReact as markdownToReact} but maybe that is the actual right answer? markdownToReact is just markdown.toReact without a dot, right?

[outoftime]

So, not a hill I want to die on, but having given it more thought I think there is a strong argument for pushing this to Redux, not in the form of “there was an error here” but rather in the form of “the instructions are known to break syntax highlighting”. So, a property of the instructions rather than the UI. WDYT?

[inlinestyle]

Well, it's not like putting it in Redux hurts at all. I don't really see a benefit, but I can do it for the sake of consistency within the project. Where in the redux state structure would you put it?

[outoftime]

Yeah I guess I see two appeals:

• Consistency, as you mention—this would be the only setState in the code base
• Also, I am a partisan of the spirit behind “everything in Redux”, particularly the claim that the UI state does derive entirely from an underlying state of the world, and that the React layer is most legible when its job is just translating that state of the world into a DOM. Obviously the ui tree in the Redux store does betray the notion that sometimes UI state is just UI state, and libraries like https://github.com/tonyhb/redux-ui and others (which I’m not inherently against) make that even more explicit. But in this case what we’re talking about is actually a fact about the instructions, namely that they can’t be rendered with the syntax highlighting code we have in place. And I do think there is value to making that explicit in the “one true state of the world”.

As for a specific proposal, I think I’d put it in the project, basically turning instructions into a record with properties along the lines of content and isKnownToBreakSyntaxHighlighting.

All that said, I’m definitely open to being told I’m wrong/being too much of a purist/etc.!

[inlinestyle]

@outoftime updated! After all that drama, it turned out that remark-react-lowlight@0.6.0 suppresses the errors. This has the added benefit that failure to recognize one code block syntax doesn't break highlighting for all code blocks in the file.

[outoftime]

Woop woop!