Regression: since 0.28.0, marked filter does not render markdown in HTML blocks

[Splaktar]

I first hit this in angular/material#11881, which ended up with the project pinning the dependency to 0.27.5 and not being able to upgrade since then. At that time, early 2020, I was quite unfamiliar with Marked and how it was used in dgeni-packages. So I was hoping that someone else would report this and it would get fixed. Unfortunately, that hasn't happened.

I have dug deeper recently in angular/angular.js#17141 and this seems to be related to 16ceb9c from PR #281, released in 0.28.0.

There is a breaking changes note:

There are a few relevant breaking changes with this latest version of `marked`.
This only affects usage of the `renderMarkdown()` service and the `marked`
nunjucks filter. Take a look through the
[marked release notes](https://github.com/markedjs/marked/releases) and
check if this affects you.

1. renderMarkdown() is not used by AngularJS or AngularJS Material.
2. They both use require('dgeni-packages/nunjucks') and the marked filter, i.e. {$ doc.description | marked $}

I read through the Marked release notes, but being unfamiliar with the library, I didn't really see anything related or of value to fixing this.

I read through this project's README.md but didn't see any references on how to use the marked filter.

It looks like the code for the filter is here:

dgeni-packages/nunjucks/rendering/filters/marked.js

Lines 1 to 13 in 0fe303e

	/**
	* @dgRenderFilter marked
	* @description Convert the value, as markdown, into HTML using the marked library
	*/
	module.exports = function markedNunjucksFilter(renderMarkdown) {
	return {
	name: 'marked',
	process(str) {
	const output = str && renderMarkdown(str);
	return output;
	}
	};
	};

I don't see how a change to any marked APIs could cause issues with this simple filter.

Except that it seems to call renderMarkdown()

dgeni-packages/nunjucks/services/renderMarkdown.js

Lines 1 to 28 in 0fe303e

	const marked = require('marked');
	/**
	* @dgService renderMarkdown
	* @description
	* Render the markdown in the given string as HTML.
	*/
	module.exports = function renderMarkdown(trimIndentation) {
	const renderer = new marked.Renderer();
	// Remove the leading whitespace from the code block before it gets to the
	// markdown code render function
	renderer.code = (code, string, language) => {
	const trimmedCode = trimIndentation(code);
	let renderedCode = marked.Renderer.prototype.code.call(renderer, trimmedCode, string, language);
	// Bug in marked - forgets to add a final newline sometimes
	if ( !/\n$/.test(renderedCode) ) {
	renderedCode += '\n';
	}
	return renderedCode;
	};
	return content => marked(content, { renderer: renderer });
	};

Which does import marked and use its APIs.

There is perhaps something that I'm missing here where this can be fixed in the AngularJS and AngularJS Material projects, but from what I can tell, with limited knowledge of this library, the fix needs to be made here.

[Splaktar]

I looked into the fact that the arguments for code() are (code, infostring, escaped) (where infostring has the lang and escaped is a boolean) but the renderMarkdown() arguments are (code, string, language). I tried

  renderer.code = (code, language, escaped) => {

    const trimmedCode = trimIndentation(code);
    let renderedCode = marked.Renderer.prototype.code.call(renderer, trimmedCode, language, escaped);

But it didn't help at all.

When I dug into this deeper, the | marked filters don't seem to be running at all. I put logs in here for code and renderedCode and some of the problematic text like [Find out more](misc/version-support-status) never came through at all.

Doh, because the problematic markdown is not code... 🤦🏼‍♂️

[Splaktar]

OK, so this doesn't parse properly:

<div class="alert alert-info">
**AngularJS Prefixes `$` and `$$`**:

To prevent accidental name collisions with your code,
AngularJS prefixes names of public objects with `$` and names of private objects with `$$`.
Please do not use the `$` or `$$` prefix in your code.
</div>

But this does

<div class="alert alert-info">

**AngularJS Prefixes `$` and `$$`**:

To prevent accidental name collisions with your code,
AngularJS prefixes names of public objects with `$` and names of private objects with `$$`.
Please do not use the `$` or `$$` prefix in your code.
</div>

[Splaktar]

Failing test

  it("should render markdown links in divs", () => {
    const html = renderMarkdown(
      '<div>\n' +
      '  some text with an important [link](https://angularjs.org)\n' +
      '</div>'
    );
    expect(html).toEqual(
      '<div>\n' +
      '  some text with an important <a href="https://angularjs.org">link</a>\n' +
      '</div>'
    );
  });

Passing test

  it("should render markdown links in divs with extra padding", () => {
    const html = renderMarkdown(
      '<div>\n\n' + // <-- this extra line feed is what makes it pass.
      '  some text with an important [link](https://angularjs.org)\n' +
      '</div>'
    );
    expect(html).toEqual(
      '<div>\n\n' +
      '<p>  some text with an important <a href="https://angularjs.org">link</a></p>\n' +
      '</div>'
    );
  });

[Splaktar]

OK, this is a bug or breaking change by design in marked. Here's a minimal demo with marked@2.0.7.

It works with marked@0.3.19 (demo) and breaks in 0.4.0.

[Splaktar]

I've opened markedjs/marked#2085 for this. It looks like this breaking change might be Working as Intended, but I hope that there is some way that I can monkey patch the parser/renderer to fix this w/o having to add a ton of blank line feeds throughout the old documentation.

[Splaktar]

So the response from the marked team was extremely short, but it basically amounted to

This was broken until 0.4.0 when we aligned with the CommonMark Spec in markedjs/marked#1135. You built all of your documentation for two major projects on top of broken functionality that we will never support. You might be able to go and write a custom renderer for markdown in block HTML elements, for which we have no real examples or guidance.

This kind of custom HTML block renderer could live in dgeni-packages, but it's not clear to me that it's viable or worth the effort. The Angular project has already moved on to remark, so there would be no benefit there.

That said, it's not ideal to leave the AngularJS and AngularJS Material projects on an old version of marked that has a huge list of major vulnerabilities. Though it could be argued that those vulnerabilities could only be triggered via commits to the repos, which are extremely limited at this time.

(╯°□°）╯︵ ┻━┻

[petebacondarwin]

The CommonMark specification is what we should be following in our use of markdown (IMO). If the only problems with upgrading to the new version of dgeni-packages is that angular.js docs need to be updated to include blank lines to trigger the switch from HTML mode to markdown mode then that is the most appropriate fix.

Note that remark (used in the Angular project) also conforms to this CommonMark specfiication.

Closing as this is working as expected in dgeni-packages.