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

renderer feature for customising output #129

Closed
wants to merge 10 commits into from

Conversation

lepture
Copy link
Contributor

@lepture lepture commented Mar 1, 2013

You can customize the result with a customized renderer.

var renderer = new marked.Renderer()

renderer.header = function(text, level) {
  return '<div class="h-' + level + '">' + text + '</div>'
}


var parse = function(src, options) {
  options = options || {};
  return marked.parser(marked.lexer(src, options), options, renderer);
}

console.log(parse('#h1'))

The renderer API:

blockcode: function(code, lang)
blockquote: function(text)
blockhtml: function(html)

header: function(text, level)
paragraph: function(text)

hrule: function()

list: function(contents, isOrdered)
listitem: function(text)

table: function(header, body)
tablerow: function(content)
tablecell: function(text, flags)
// flags: {header: false, align: 'center'}

I am doing my best to make the renderer API comptable with sundown's renderer.

This is the feature I really need. If marked has, I can drop the c binding sundown of robotskirt.

@lepture
Copy link
Contributor Author

lepture commented Mar 1, 2013

There are 4 failed test cases, and the master branch failed too.

@lepture
Copy link
Contributor Author

lepture commented Mar 1, 2013

@lepture
Copy link
Contributor Author

lepture commented Mar 5, 2013

ping ...

@rhiokim
Copy link

rhiokim commented Mar 11, 2013

@lepture interesting your work :-)
I did testing based on your code. But it's not working

on node v0.8.16 & browser too

var marked = require('./index');
var renderer = new marked.Renderer();

renderer.header = function(text, level) {
   return '<div class="h-' + level + '">' + text + '</div>';
};

var parser = new marked.Parser(marked.defaults, renderer);
console.log(parser.parse('# h1'));

Error: Tokens array requires a `links` property.
    at new InlineLexer (~/Works/test/marked/lib/marked.js:517:11)
    at Parser.parse (~/Works/test/marked/lib/marked.js:833:17)
    at repl:1:20
    at REPLServer.self.eval (repl.js:109:21)
    at rli.on.self.bufferedCmd (repl.js:258:20)
    at REPLServer.self.eval (repl.js:116:5)
    at Interface.<anonymous> (repl.js:248:12)
    at Interface.EventEmitter.emit (events.js:96:17)
    at Interface._onLine (readline.js:200:10)
    at Interface._line (readline.js:518:8)

And README.md example on renderer branch in your github repo too

var renderer = new marked.Renderer();
renderer.blockcode = function(code, lang) {
  if (!lang) {
    return '<pre><code>' + code + '</code></pre>';
  }
  return highlight(code, lang);
};
renderer.header = function(text, level) {
  return '<div class="h-' + level + '">' + text + '</div>';
};

var parse = function(src, options) {
  marked.parser(marked.lexer(src, options), options, renderer);
}
console.log(parse('# h1'));

@lepture
Copy link
Contributor Author

lepture commented Mar 12, 2013

@rhiokim the example is not clearly. I've updated readme and this issue. try this:

var marked = require('./index')

var renderer = new marked.Renderer()

renderer.header = function(text, level) {
  return '<div class="h-' + level + '">' + text + '</div>'
}


var parse = function(src, options) {
  options = options || {};
  return marked.parser(marked.lexer(src, options), options, renderer);
}

console.log(parse('#h1'))

@rhiokim
Copy link

rhiokim commented Mar 12, 2013

@lepture thanks :)

@lepture
Copy link
Contributor Author

lepture commented Mar 18, 2013

ping ....

@3rd-Eden
Copy link

do want.

@lepture
Copy link
Contributor Author

lepture commented Mar 22, 2013

@chjj Has any idea?

@chjj
Copy link
Member

chjj commented Mar 22, 2013

@lepture, have you benchmarked this? I've considered this before, but the overhead of so many function calls scares me. It doesn't seem like it would be a lot, but when you're calling those functions several thousands of times, it can add a lot of overhead. It all depends on whether v8 does some kind of function inlining optimization there. If the benchmarks check out, I'll merge it.

@lepture
Copy link
Contributor Author

lepture commented Mar 23, 2013

@chjj Yes, I did the benchmark, it is slower, but not much. The convenience worth it.

Before this pull request:

# test 1
marked completed in 3833ms.
marked (gfm) completed in 3971ms.
marked (pedantic) completed in 3384ms.
# test 2
marked completed in 3726ms.
marked (gfm) completed in 3942ms.
marked (pedantic) completed in 3364ms.
# test 3
marked completed in 3733ms.
marked (gfm) completed in 3922ms.
marked (pedantic) completed in 3379ms.

With this pull request:

# test 1
marked completed in 3727ms.
marked (gfm) completed in 3952ms.
marked (pedantic) completed in 3407ms.
# test 2
marked completed in 3830ms.
marked (gfm) completed in 4052ms.
marked (pedantic) completed in 3480ms.
# test 3
marked completed in 3776ms.
marked (gfm) completed in 3934ms.
marked (pedantic) completed in 3422ms.

On my Macbook Pro. Node v0.10.0

@kuba-kubula
Copy link

👍 +1 here, would be great for marked to have plugins, because this looks like plugging into marked

@lepture
Copy link
Contributor Author

lepture commented Mar 26, 2013

@chjj ping ...

@xixixao
Copy link

xixixao commented Mar 26, 2013

+1 (github-like anchors being motivation)

@jorilallo
Copy link

I would really like to see this being merged. If it won't be included, it would be great to have an actively updated fork which would be more feature rich - I see more use for feature rich than super fast Markdown converter.

@jonschlinkert
Copy link

I see more use for feature rich than super fast Markdown converter.

Completely agree. I think the speed objective is outdated considering that 95% of users are only building a couple of pages, and the people who are converting more than a few pages need more features, 2) marked is usually only a small piece of a larger picture, where features are more important, 3) I've never seen a request to make marked faster (I'm not saying there hasn't been), 4) users keep saying they want these features.

@aleemb
Copy link

aleemb commented Nov 27, 2013

@lepture think you could implement the span-level features to make this feature-complete?

@lepture
Copy link
Contributor Author

lepture commented Nov 27, 2013

I sent this patch months ago. At that time, this.options.headerPrefix was not added. The renderer feature can do the same thing.

@aleemb I need to make sure that this renderer feature is accepted before adding more features.

@lepture
Copy link
Contributor Author

lepture commented Dec 3, 2013

I've created a forked project: https://github.com/lepture/markit

@lepture
Copy link
Contributor Author

lepture commented Dec 3, 2013

Close now.

@chjj when you are ready to accept this feature, please tell me. I would send another pull request.

For now. I am going to maintain https://github.com/lepture/markit

@lepture lepture closed this Dec 3, 2013
@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

It's not that slow. Here is the benchmark: https://github.com/lepture/markit#benchmark

Here are the results:

markit completed in 3330ms.
markit (gfm) completed in 3356ms.
markit (pedantic) completed in 2897ms.
marked completed in 3181ms.
marked (gfm) completed in 3327ms.
marked (pedantic) completed in 2908ms.
robotskirt completed in 722ms.
showdown (reuse converter) completed in 9926ms.
showdown (new converter) completed in 17152ms.
markdown.js completed in 13733ms.
markit completed in 3290ms.
markit (gfm) completed in 3356ms.
markit (pedantic) completed in 2874ms.
marked completed in 3097ms.
marked (gfm) completed in 3268ms.
marked (pedantic) completed in 2879ms.
robotskirt completed in 721ms.
showdown (reuse converter) completed in 10064ms.
showdown (new converter) completed in 14582ms.
markdown.js completed in 12390ms.
markit completed in 3263ms.
markit (gfm) completed in 3377ms.
markit (pedantic) completed in 2835ms.
marked completed in 3135ms.
marked (gfm) completed in 3279ms.
marked (pedantic) completed in 2842ms.
robotskirt completed in 710ms.
showdown (reuse converter) completed in 11228ms.
showdown (new converter) completed in 17194ms.
markdown.js completed in 13445ms.

@chjj
Copy link
Member

chjj commented Dec 4, 2013

I'm very sorry. Will accept. I've been absorbed in other things lately. Merging and refactoring now.

@chjj
Copy link
Member

chjj commented Dec 4, 2013

Merged with 7eed150

All tests passing.

@aleemb
Copy link

aleemb commented Dec 4, 2013

I suppose there was an overwhelming demand for this. 59 stars on lepture/markit in one day. Glad it's sorted.

@chjj
Copy link
Member

chjj commented Dec 4, 2013

Yeah, well. This is me breaking out of my shell. I believe the thing that made marked successful in the first place was the attention put on speed. marked does so many weird optimizations that I'm worried only I understand (not because I'm smart, but because I wrote them and I do stupidly-designed optimizations that no sane programmer would do).

This is the largest pull request I've ever accepted for marked. So while it's partially true that I've been busy with work and couldn't accept this PR, it's also partially true that I've trained myself to be hesitant about large PRs: I'm worried people may make changes without realizing they're overlooking some esoteric optimization. I wrote marked while running benchmarks after every single change to a line. If I lose a few milliseconds, it's a tragedy to me. I've come to terms with the fact that a renderer is a good idea (much thanks to @lepture for his contribution). Hopefully we can do the same thing with the lexer(s) without too much overhead.

I've also just added @ChrisWren as a core committer for documentation. It's been an eventful night for marked.

@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

Actually, I don't think headerPrefix is a good idea.

@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

@chjj I've already added renderer in InlineLexer. https://github.com/lepture/markit

I think you shouldn't merge this patch. Maybe I can send you another clean one.

@jonschlinkert
Copy link

I don't think headerPrefix is a good idea.

Agreed. It's not necessary with this

@chjj
Copy link
Member

chjj commented Dec 4, 2013

@lepture, I imagine you would prefer that functionality to be handled by the renderer? We can remove it. Being that it was never officially released in an npm verson, we arguably don't have to maintain backward compat. Although, I do worry about a lot of people who simply grab the latest HEAD for their clientside app.

Anyway, maybe we could open a new issue for this.

@chjj
Copy link
Member

chjj commented Dec 4, 2013

Also, if nothing else, we should definitely pass the raw text to the renderer function, since the text handled by the inline lexer is a bit harder to sanitize.

@chjj
Copy link
Member

chjj commented Dec 4, 2013

@lepture, open another pull request for this and the inline renderer. I'll take a look.

@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

@chjj I will start it at 77352ec

lepture added a commit to lepture/marked that referenced this pull request Dec 4, 2013
At markedjs#129, it only contains block level renderers. This patch contains
both block level and span level renderers.
Since renderer feature can do many things, I removed highlight option
and headerPrefix option.

markedjs#129
@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

@chjj could you please create a new branch at 77352ec

I will send it to this branch. The master branch is a little mess.

@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

@lepture
Copy link
Contributor Author

lepture commented Dec 4, 2013

marked does so many weird optimizations that I'm worried only I understand

I think you did it very well. The code is very pretty, and easy to understand. I have looked into markdown.js, which is a disaster, I can't figure out where to begin.

@chjj
Copy link
Member

chjj commented Dec 4, 2013

I think you did it very well. The code is very pretty, and easy to understand. I have looked into markdown.js, which is a disaster, I can't figure out where to begin.

Thanks. That's good to hear.

I have a branch up at for_lepture if that helps.

@lepture lepture mentioned this pull request Dec 4, 2013
@lepture lepture deleted the renderer branch March 12, 2014 09:55
ghost pushed a commit to zergeborg/marked that referenced this pull request May 13, 2016
At markedjs#129, it only contains block level renderers. This patch contains
both block level and span level renderers.
Since renderer feature can do many things, I removed highlight option
and headerPrefix option.

markedjs#129
svn2github pushed a commit to svn2github/marked that referenced this pull request Jul 1, 2017
At #129, it only contains block level renderers. This patch contains
both block level and span level renderers.
Since renderer feature can do many things, I removed highlight option
and headerPrefix option.

markedjs/marked#129


git-svn-id: https://github.com/chjj/marked.git@549 de94abd1-4c94-9ff3-2ac9-ce57b7db7bff
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