Add slugger option, remove headerIds / headerPrefix

[styfle]

Marked version: 0.5.1

Markdown flavor: All

Description

Fixes #1280 by adding a slugger option so the user can define how <h1 id=""> is generated.

This also removes the headerIds and headerPrefix option since the slugger option can handle both use cases.

Request for comments

• Q1: Should we provide a default Slugger implementation or just recommend the github-slugger package? (we would have to modify unit tests)
  • A1: No default, because it's not part of the spec. Completely removed Slugger implementation and fixed tests.
• Q2: Any reason to keep the headerIds or headerPrefix option?
  • A2: No, these can be implemented with a Slugger implementation in a few lines of code (seen below).

Usage

function Slugger () {
  this.seen = {};
}

Slugger.prototype.slug = function (raw) {
  var slug = raw.toLowerCase().replace(/[^\w]+/g, '-');
  var count = this.seen.hasOwnProperty(slug) ? this.seen[slug] + 1 : 0;
  this.seen[slug] = count;

  if (count > 0) {
    slug = slug + '-' + count;
  }

  return slug;
};

var options = { slugger: new Slugger() };
var md = `# heading
## heading
# heading again
## heading again`;
var html = marked(md, options);

Try it

https://runkit.com/embed/1cawn0orvsca

Contributor

• Tests exist to ensure functionality and minimize regression
• If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

• Draft GitHub release notes have been updated.
• CI is green (no forced merge required).
• Merge PR

[Martii]

Don't mind this please... just a little easier to see what was changed (fewer clicks):

• https://github.com/markedjs/marked/pull/1354/files?w=1

[styfle]

@Martii Oh my, I am embarrassed 😅

I fixed the whitespace issues, thanks! 👍

Note that I haven't fixed the unit tests yet because I wanted to get some early feedback on this (see original post).

[UziTech]

I think this should be an extension. It would be really nice to eventually remove all options and just have extensions.

[Grawl]

Any reason to keep the headerIds or headerPrefix option?

What if I have <div id='header'> in my template and I have to render a markdown with # Header? Slugger's scope is markdown file, not any thing outside it. We need a prefix here to make markdown scope safe.

[styfle]

@UziTech I think I prefer using an extension too, I imagine most users will use github-slugger and that's one less thing we need to maintain.

Should I modify the unit tests that are failing so they no longer look for a id attribute?

[styfle]

@Grawl Since the user provides the Slugger implementation, they could hard-code the prefix into their custom slugger.

Something like the following

var GithubSlugger = require("github-slugger");
var ghslugger = new GithubSlugger();

function PrefixSlugger () {
  this.headerPrefix = 'MARKED-PREFIX-';
}
PrefixSlugger.prototype.slug = function (value) {
  return this.headerPrefix + ghslugger.slug(value);
};

var options = { slugger: new PrefixSlugger() };

Try it here: https://runkit.com/embed/jzea41wp28cc

[Grawl]

cool but better to make out of the box i think

[UziTech]

@Grawl The problem with integrating it is that there are thousands of things that could be integrated (header ids, LaTex grammer, highlighting, sanitizing, etc.) and we don't want to maintain thousands of options. We just want to convert markdown to HTML according to the spec in a fast and secure way. Which is why it would be better to create an easy way to extend marked so the community can maintain whichever extensions they want.

[Grawl]

Guarantee of unique IDs is not an option.

[styfle]

@joshbruce @UziTech @davisjam I updated the docs so this PR is ready to be reviewed.

[UziTech]

I still feel like this would be a breaking change that is not worth the trouble if our goal is to get rid of this option at some point.

I feel like a better fix for this would just be to have a section in the docs on how to extend the renderer to use github-slugger.

[styfle]

@UziTech By extend, you mean override the renderer like I did here?

That seems a little hacky/brittle for a feature that is probably widely used (it is currently enabled by default in marked).

Could we get more people to comment on this? @joshbruce @davisjam @Feder1co5oave

[UziTech]

I was thinking more like this.

Using the renderer option to extend the renderer the way it shows in the docs.

[UziTech]

I would like to eventually have no options (or as few as possible) and have different modules that add this functionality. Something like:

// marked-slugger module

const Slugger = require("github-slugger");
const slugger = new Slugger();

function markedSlugger(marked) {
  // extend renderer
  const renderer = new marked.Renderer();
  renderer.heading = function(text, level, raw) {
    const slug = slugger.slug(raw);
    return `<h${level} id="${slug}">${text}</h${level}>\n`;
  };
  
  const options = { renderer };
  marked.setOptions(options);

	return marked;
}

module.exports = markedSlugger;

---

// in their module

const marked = require("marked");
marked = require("marked-slugger")(marked);
marked = require("marked-other-module")(marked);

// use marked with plugins
marked(...)

That way their can be lots of community or markedjs maintained modules that don't mess up the marked code base.

[joshbruce]

The feature is widely used because it is on by default. I think we added the ability to turn it off recently - think I did it.

To @UziTech's point, while going through the history of tickets we decided to get out of the header ID at some point - version 2.x, I think.

Most of the errors related to the issue are from glyph-based languages. Further, given the HTML rules around the uniqueness of IDs on an entire page, it's definitely a good reason to opt for the extending path, imho.

If it could be easily done, without breaking changes, in a way that would be easy to remove later, I'd say it might be worth it.

[styfle]

It's a pretty minimal api that accepts a single parameter so I don't think we're taking on a huge maintenance burden see diff.

@UziTech I think you're solution will actually lead to more maintenance because now we have to maintain multiple plugin packages and version then appropriately. Then communicate that via docs. Unless we expect the community to really standup here but there hasn't been much interaction so I don't expect this.

The whole purpose of this PR was to fix 3 bugs:

• colliding IDs when using the same heading
• unicode in heading causing invalid ID
• header IDs by default is not spec compliant

I would like to eventually have no options (or as few as possible) and have different modules that add this functionality.

If we really want to go this route, maybe we should figure out which options can be implemented externally so that we know what our future goals are and what the minimum set of options that marked should expose could be.

[UziTech]

The biggest reason why the plugin system works is because it allows the community to come up with these fixes without needing the core module to approve it. And each module would ideally be small (like my marked-slugger example above) so the maintenance would be small.

This code wouldn't be hard to maintain but it would be sending the wrong message to the community, that we are going to be adding options to marked, especially if we are just going to remove it in the future.

Anyone who wants header ids (which I assume is a lot of people otherwise this PR doesn't really matter anyways) is going to have to change their code after this PR is merged and again after the slugger option is removed. It would be easier for them to just extend the renderer in the first place.

I would support a PR that fixes those issues without changing the options. (i.e. implement github-slugger internally)

[joshbruce]

If we really want to go this route, maybe we should figure out which options can be implemented externally so that we know what our future goals are and what the minimum set of options that marked should expose could be.

See #1080 as I started working toward that. Note, Header IDs is under the part to deprecate.

Unless we expect the community to really standup here but there hasn't been much interaction so I don't expect this.

I agree. Having said that, we've also been operating under the idea that it doesn't matter how long stuff takes. Unless we're wanting to shift gears, I appreciate the tone and participation of the community since adopting a more, if you find it, please fix it approach and letting the core manage the management, so to speak. If we, as the core, are recommending this PR, we should talk more because it does alter the current path.

This code wouldn't be hard to maintain but it would be sending the wrong message to the community, that we are going to be adding options to marked, especially if we are just going to remove it in the future.

Agreed.

I would support a PR that fixes those issues without changing the options. (i.e. implement github-slugger internally)

Agreed. See #1114

[Martii]

@UziTech Cc: @joshbruce

It would be easier for them to just extend the renderer in the first place.

Which is similar to what we did very long ago somewhere around 2014... albeit with no unique id checking but that can be remedied perhaps with github-slugger or mirroring those changes into our older routine that conceptually does exactly what your code snippet suggests.

[styfle]

All,

Thanks for the feedback.

I realize now that I misinterpreted the original comment

I think this should be an extension. It would be really nice to eventually remove all options and just have extensions.

@UziTech When I read "extension", I assumed you were answering my question in the PR description about implementing our our slugger or recommending the extension github-slugger. So I went with the second one. Now I understand that you are saying we shouldn't touch the options at all unless we are removing them.

The problem with your extension example here is that the extensions would not be composable (if you need two extensions).

let marked = require("marked");
marked = require("marked-slugger")(marked); // override renderer
marked = require("marked-other-module")(marked); // override renderer again (oops)

[Feder1co5oave]

My early idea of designing an extension system was to use the Proxy design pattern to allow modules to modify parameters before rendering to html (for example, rebasing href parameters onto a specified basePath):

renderer = new marked.Renderer()
renderer.link = function (href, title, text) {
  return renderer.__parent__.link(href.replace(/^\//, basePath), title, text);
}

and use a pseudo-DOM (e.g. ReactDOM) to easily modify the html AST before translating it into its textual representation. For example, open external links to a new window:

renderer = new marked.Renderer()
renderer.link = function(href, title, text) {
  anchor = renderer.__parent__.link(href, title, text);
  if (href.match(externalLinkRegex))
    anchor.addAttribute('target', '_blank'); // use pseudo-DOM methods to alter html
  return anchor;
}

I believe I wrote some draft of this a long time ago, and the sole use of ReactDOM made the conversion process twice as slow. I'll see if I can found the WIP branch.

[UziTech]

@styfle Correct, we would have to figure out a better way to compose plugins and allow more of marked to be extendable.

[styfle]

Closing in favor of #1401