Markdown to HTML

Question

String.prototype.replaceAll = function(find, replace) {
    if (typeof find == 'string') return this.split(find).join(replace);
    var t = this, i, j;
    while (typeof(i = find.shift()) == 'string' && typeof(j = replace.shift()) == 'string') t = t.replaceAll(i || '', j || '');
    return t;
};

function html(input) {
    return input.toString().replaceAll(['&', '<', '"'], ['&amp;', '&lt;', '&quot;']);
};
function markdown(src) {
    var h = '',
        i = 0;
    function inlineEscape(s) {
        return html(s)
            .replace(/!\[([^\]]*)]\(([^(]+)\)/g, '<img alt="$1" src="$2">')
            .replace(/\[([^\]]+)]\(([^(]+)\)/g, '$1'.link('$2'))
            .replace(/([^"])(https?:\/\/([^\s"]+))/g, '$1$3'.link('$1$2'))
            .replace(/^(https?:\/\/([^\s"]+))/g, '$2'.link('$1'))
            .replace(/`([^`]+)`/g, '<code>$1</code>')
            .replace(/\*\*([^*]+)\*\*/g, '<strong>$1</strong>')
            .replace(/\*([^*]+)\*/g, '<em>$1</em>');
    }
    src.replace(/\r|\s+$/g, '').replace(/\t/g, '    ').split(/\n\n+/).forEach(function(b, f, R) {
        f = b.substr(0, 2);
        R = {
            '* ': [(/\n\* /), '<ul><li>', '</li></ul>'],
            '- ': [(/\n- /), '<ul><li>', '</li></ul>'],
            '  ': [(/\n    /),'<pre><code>', '</code></pre>', '\n'],
            '> ': [(/\n> /),'<blockquote>', '</blockquote>', '\n']
        }[f];
        if (b.match(/\n[1-9]\d*\. /)) R = [(/\n[1-9]\d*\. /), '<ol><li>', '</li></ol>'];
        if (b.match(/\n[1-9]\d*\) /)) R = [(/\n[1-9]\d*\) /), '<ol><li>', '</li></ol>'];
        f = b[0];
        if (R) h += R[1] + ('\n' + b).split(R[0]).slice(1).map(R[3] ? html : inlineEscape).join(R[3] || '</li>\n<li>') + R[2];
        else if (f == '#') h += '<h' + Math.min(6, f = b.indexOf(' ')) + '>' + inlineEscape(b.slice(f + 1)) + '</h' + Math.min(6, f) + '>';
        else {
            h += '<p>' + inlineEscape(b) + '</p>';
            i++;
        }
    });
    if (i == 1) return inlineEscape(src);
    return h;
};

• The string prototype lets me do things like:

  fileContents.replaceAll(['$mythingtoreplace1', '$mythingtoreplace2'], ['data1', 'data2'])
  

  I know modifying native object prototypes is controversial…

• html() is a html escape.

• markdown() is a markdown -> html converter.

Since this will be injected into the page, it must be secure. Not sure about my regexes…

.replace(/([^"])(https?:\/\/([^\s"]+))/g, '$1$3'.link('$1$2'))
.replace(/^(https?:\/\/([^\s"]+))/g, '$2'.link('$1'))

^ This also seems kind of wrong.

What can I improve here?

Answer 1

Overall, this looks like pretty good JavaScript code.

I would have two comments though, to avoid errors, long term (i.e. maintainability):

1. Direct Statement

   if(expr) statement;
   

   This is correct in all C-like languages, unfortunately. Only, if you write two statements after the if() and forget to add the curly brackets, you end up with a bug. I always write things like this:

   if(expr)
   {
       statement;
   }
   

   and avoid tons of bugs (with time).

2. Assignments "in the wrong place"

   I noticed that you were assigning values within expressions and if() or while() statements.

   For example:

   while (typeof(i = find.shift()) == 'string' && typeof(j = replace.shift()) == 'string') ...
   

   This looks good, but it is difficult to read. You could say "maybe for bad programmers", but remember that you do not know who will maintain your code...

   In such a case, I'd use a forever loop and exit as required:

   for(;;)
   {
       i = find.shift();
       j = replace.shift();
       if(typeof i != 'string' || typeof j != 'string')
       {
           break;
       }
       // ...
   }
   

   Now, a programmer who cannot read that is probably not a programmer at all.

   Note: I know this is JavaScript and people try to pre-optimize (i.e. removing as many characters as possible) but JavaScript compressors are here to do that for us. They will remove the { and } if not necessary, and very advanced compressors (like the closure compiler from Google) will go way further and possibly optimize that back to a while() if their optimizer is strong enough.

---

The security will depend on the source of the data. If it is just the client (as Stack Overflow does while you are typing) then it does not matter at all since the client is responsible for his own introduced XSS code. Genuine users will generally not have any problems unless they do a copy and paste from a totally unsafe source.

Once the data gets saved on the server, you can then verify that the data is safe before sending it back out to someone else. (i.e. URIs could be checked against a black list of known hacker sites.)

---

One comment I'd make is that the > character is also considered an HTML entity and it should be converted to >. This means the quoted test should use the string '> ' (in the R map).

---

I find it really strange when people use <pre> and <code> together. It is legal, although <code> is an inline element. I would suggest you just use <pre> and if necessary add one or more classes to better style the block.

---

In regard to this one replace() call:

.replace(/([^"])(https?:\/\/([^\s"]+))/g, '$1$3'.link('$1$2'))

It seems to me that $1 is not part of the URI, so you should have something more like '$1' + '$3'.link('$2').

(See more below about the double quote.)

---

For the previous one and the following:

.replace(/^(https?:\/\/([^\s"]+))/g, '$2'.link('$1'))

I wonder why you check the double quote at the end. If this is because a double quote should never appear in a URI, then you are missing a few other characters that are expected to be invalid in URIs.

The & and " should be escaped, but they can be included in URIs:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/link

However, your html() call earlier already transformed those two entities so the match will never happen anyway. The URI is now possibly written between " and not ".

---

In your markdown() function you have a statement with a function declaration like this:

.forEach(function(b, f, R) {
   f = ...;
   R = ...;

The f and R variables must be defined as variable inside that function and not as parameters since you overwrite them otherwise.

So it should be:

 .forEach(function(b) {
   var f = ...,
       R = ...;

Note that you do it properly in the other functions!

---

For the following two statements, I would add !R because if R is already defined, you do not need to waste your time testing for a match;

if (!R && b.match(/\n[1-9]\d*\. /)) R = ...;
if (!R && b.match(/\n[1-9]\d*\) /)) R = ...;

Note also that 1.test will not match. I suppose that's on purpose.

---

I think that one is incorrect:

if (i == 1) return inlineEscape(src);

If I understand properly, if you just find one paragraph then you return a "simple" inlineEscape() of the source instead of all the other possibly changes. Only you probably miss many other things (i.e. if you have one paragraph followed by a list, i will be 1 and the result will most certainly be wrong.)

Answer 2

Not sure where to start..

First off, you might want to look at this response as to whether using regexes for parsing markdown to HTML is safe. You are of course not parsing HTML, but the sentiment is still valid in my mind.

Second off, there is this and this on github, these parsers are a few hundred lines. You are missing use cases in your 30 lines of code. That is fine for a hobby site, not for a production site.

If you have doubts, you can always run MD tests on your code and see for your self.

Other than that;

• If you insist on using regexes, at the very least add a line of comment of what you are trying to achieve, ideally add a second line with an example
• Modifying standard objects, enough said..
• Single letter variables ( f = b.substr(0, 2); ) are very unfortunate
• 0 lines of comment together with the previous comment make for terrible code
• html() -> If you learn 1 thing today, it should be that mere mortals (like you and me) should not write escapers or parsers from scratch, instead we should figure out how smarter people did it and re-use their work. In jQuery, the authors call the DOM method .createTextNode(), which replaces special characters with their HTML entity equivalents. If you are running this server side as per your comment, then I would suggest you look up how Chromium does it. You did well and only forgot about { '\'', "'" },

• Assignments in if statements are considered bad form:

  while (typeof(i = find.shift()) == 'string' && typeof(j = replace.shift()) == 'string') t = t.replaceAll(i || '', j || '');
  

  To turn this into good code, you will have to roll this out to something like

  while ( find.length ){
    caterpillar = find.shift();
    butterfly = replace.shift();
    if( typeof caterpillar != 'string' || typeof butterfly != 'string' )
      break;
    //Turn all caterpillars into butterflies
    this = this.replaceAll( caterpillar , butterfly );
  }
  

All in all, you are re-inventing the wheel. If you want to continue with this code then you have your work cut out..