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: Remove oniguruma dependency by replacing marky-markdown by markdown-it #73

Closed
wants to merge 7 commits into from

Conversation

elingerojo
Copy link
Member

@elingerojo elingerojo commented May 2, 2017

WIP: do not merge

This PR is just an option to solve de Node 4 support.

  • Changed marky-markdown dependency to markdown-it + markdown-it-named-headers + highlight.js

@elingerojo
Copy link
Member Author

elingerojo commented May 4, 2017

WIP label removed.

Now this PR is ready to merge (if we decide it is the appropriate option for solving the Node 4 support problem)

  • Changed marky-markdown dependency to markdown-it + highlight.js
  • Added a test for syntax highlighting fenced code
  • Removed markdown-it-named-headers for backwards compatibility
  • Updated README dependencies and duplicated build badge
  • Bumped version 0.25.0

@elingerojo elingerojo changed the title WIP: Remove oniguruma dependency by replacing marky-markdown by markdown-it Remove oniguruma dependency by replacing marky-markdown by markdown-it May 4, 2017
@jdormit
Copy link
Member

jdormit commented May 4, 2017

I'm happy with this solution if the rest of you all are!

I did some brief testing - I can't seem to get syntax highlighting for code blocks to work. I tried GitHub markdown syntax, e.g.

```javascript
console.log("foo")
````

I also tried it without the language specifier, seeing if it would autodetect. I can't find any documentation in either markdown-it or highlight.js that specifies how the language is detected - am I doing something wrong or is it not working properly?

@elingerojo
Copy link
Member Author

elingerojo commented May 4, 2017

@jdormit
There are two suggested ways by markdown-it docs.
I selected one, maybe I picked the wrong one for jus

1) Selected one

// Actual default values
var md = require('markdown-it')({
  highlight: function (str, lang) {
    if (lang && hljs.getLanguage(lang)) {
      try {
        return hljs.highlight(lang, str).value;
      } catch (__) {}
    }

    return ''; // use external default escaping
  }
});

2) Not selected one (user needs to assign class to <pre>)

// Actual default values
var md = require('markdown-it')({
  highlight: function (str, lang) {
    if (lang && hljs.getLanguage(lang)) {
      try {
        return '<pre class="hljs"><code>' +
               hljs.highlight(lang, str, true).value +
               '</code></pre>';
      } catch (__) {}
    }

    return '<pre class="hljs"><code>' + md.utils.escapeHtml(str) + '</code></pre>';
  }
});

For <pre> class you need to do something like this (as suggested here):

<link rel="stylesheet" href="//cdnjs.cloudflare.com/ajax/libs/highlight.js/9.11.0/styles/default.min.css">

What's the correct one for jus?

I don't know.

  • Needs to be backwards compatible so we don't break anything

If you want, I can do the change and you test it because I am not sure I am getting the problem right.

The way I saw it, this works (with out particular language highlight)

    ```
    console.log("foo")
    ```

...but as you mentioned, this does not work (and it should work)

    ```javascript
    console.log("foo")
    ```

Question: Did it use to work in jus? (I hadn't use any fenced code with jus so the question)

...or maybe, marky-markdown could not be exactly replaced by an options combination with markdown-it as we are trying to do here.

@elingerojo elingerojo changed the title Remove oniguruma dependency by replacing marky-markdown by markdown-it WIP: Remove oniguruma dependency by replacing marky-markdown by markdown-it May 4, 2017
@jdormit
Copy link
Member

jdormit commented May 4, 2017

@zeke would be better placed to answer that question - I never did any syntax highlighted code blocks with jus, so I'm not sure if it was possible before.

@zeke
Copy link
Member

zeke commented May 4, 2017

I would prefer not to move off of marky-markdown. I wrote it for the npm website, and it's been battle-tested over the years and proven to support hundreds of thousands of readmes.

When first writing marky-markdown I actually started with highlight.js, but it turned out to be buggy and error-prone. Then we tried pygments which worked well but has a python dependency. Eventually we settled on highlights which is used by atom. That's where the oniguruma dependency comes from.

I would rather just get atom/node-oniguruma#66 sorted out instead.

@zeke
Copy link
Member

zeke commented May 4, 2017

Did it use to work in jus?

Yes. See http://zeke.sikelianos.com/electron-nativefier/

@elingerojo
Copy link
Member Author

I would prefer not to move off of marky-markdown

You are right. I did the tests and markdown-it does things differently. It won't work for us because it breaks jus.

...well, back to square one.


Just as a reference, here are the tests results,

Tests

Code sample 1

```
function test() {
  console.log("foo");
}
```

Code sample 2

```javascript
function test() {
  console.log("foo");
}
```

Results - jus with original marky-markdown

Code sample 1

<div class="highlight ">
  <pre class="editor editor-colors">
    <div class="line">
      <span class="text plain null-grammar">
        <span>function&#xA0;test()&#xA0;{</span>
	...

Code sample 2

<div class="highlight javascript">
  <pre class="editor editor-colors">
    <div class="line">
      <span class="source js">
        <span class="meta function js">
          <span class="storage type function js">
            <span>function</span>
          </span>
          <span>&#xA0;</span>
          <span class="entity name function js">
            <span>test</span>
          </span>
	...

Results - jus with new markdown-it with suggested highlight code one

Code sample 1

<pre class="hljs">
  <code>
function test() {
  console.log(&quot;foo&quot;);
}
  </code>
</pre>

Code sample 2

<pre>
  <code class="language-javascript">
    <span class="hljs-function">
      <span class="hljs-keyword">function</span> 
      <span class="hljs-title">test</span>
	...

Results - jus with new markdown-it with suggested highlight code two

Code sample 1

<pre class="hljs">
  <code>
function test() {
  console.log(&quot;foo&quot;);
}
  </code>
</pre>

Code sample 2

<pre class="hljs">
  <code>
    <span class="hljs-function">
      <span class="hljs-keyword">function</span> 
      <span class="hljs-title">test</span>
	...

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.

None yet

3 participants