elm-markdown icon indicating copy to clipboard operation
elm-markdown copied to clipboard

Replaced marked with markdown-it

Open liamcurry opened this issue 8 years ago • 4 comments

marked hasn't been updated in nearly a year, is looking unmaintained, and was just found to be vulnerable to XSS attacks.

This PR replaces marked with markdown-it, a newer, actively maintained Markdown parser based on remarkable. markdown-it conforms to the CommonMark spec, but also supports Github flavored Markdown stuff like tables and code fencing (disabled by default). It's also faster than marked according to their benchmarks.

The changes in this PR are mainly replacing the marked library with markdown-it, and tweaking the Options for this new parser. Here are the options markdown-it supports natively for reference.

One non-trivial change (from a users perspective) is enabling input sanitization by default (which is now behind the html = False flag). It's very easy to forget about enabling sanitization with custom options, and I think having it on by default is worth avoiding the risk of accidentally releasing XSS exploitable code into production. I'd like to hear what you all think about this.

Feedback welcome. I'm happy to make nitpick/stylistic changes as well.

Should fix #6, #9, #16, and #17.

liamcurry avatar May 19 '16 05:05 liamcurry

I've left some comments. Generally, it's best practice to not just copy whatever JS libaries do. They use bad names and bad data structures. Think about the perspective of the Elm user. Names should be clear, and sensible. You can convert a option record to a record that matches the markdown library after the Elm programmer has touched it. Make life easier for the Elm programmer, not for the JS lib.

eeue56 avatar May 19 '16 14:05 eeue56

"full" enables all of what markdown-it has to offer. I didn't stay with "githubFlavored" because markdown-it offers more than that I think. But "githubFlavored" would still probably be a better/more accurate name than "full".

For the quotes, markdown-it allows you to change the "smart" quotes that it adds if you've enabled that option. To change them, you can use a string 4 characters wide, e.g. “”‘’, or an array of 4 characters, ['“', '”', '‘', '’']. To be more precise and avoid runtime errors I thought it'd be better to use a small record, where "dl" = double quote left, "dr" = double quote right, "sl" = single quote left, and "sr" = single quote right. I suppose I could make them a little longer, to something like "doubleLeft", "doubleRight", "singleLeft", and "singleRight". Would that work?

liamcurry avatar May 19 '16 14:05 liamcurry

naming is hard! githubFlavoured would keep the API consistent, so I would probably stick with that. Or withExtras or something.

👍 much better names for the quote options

eeue56 avatar May 19 '16 14:05 eeue56

Just updated the PR with some better names. Any other issues?

liamcurry avatar May 19 '16 14:05 liamcurry