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.

Sign up for GitHub

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

anchor-markdown-headings.js: fix IDs to be unique. #2441

Merged
merged 2 commits into from
Sep 2, 2019
Merged

anchor-markdown-headings.js: fix IDs to be unique. #2441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2fa6e1b
anchor-markdown-headings.js: fix IDs to be unique.
XhmikosR Aug 21, 2019
af482f4
Fix duplicate IDs in Italian buffer-constructor-deprecation.md.
XhmikosR Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions locale/it/docs/guides/buffer-constructor-deprecation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ layout: docs.hbs

# Porting to the `Buffer.from()`/`Buffer.alloc()` API

<a id="overview"></a>
## Overview

This guide explains how to migrate to safe `Buffer` constructor methods. The migration fixes the following deprecation warning:
Expand Down Expand Up @@ -60,8 +59,7 @@ There is a drawback, though, that it doesn't always
overridden e.g. with a polyfill, so recommended is a combination of this and some other method
described above.

<a id="variant-1"></a>
## Variant 1: Drop support for Node.js ≤ 4.4.x and 5.0.0 — 5.9.x
## <!--variant-1-->Variant 1: Drop support for Node.js ≤ 4.4.x and 5.0.0 — 5.9.x

This is the recommended solution nowadays that would imply only minimal overhead.

Expand Down Expand Up @@ -91,8 +89,7 @@ or [Variant 3](#variant-3) on older branches, so people using those older branch
the fix. That way, you will eradicate potential issues caused by unguarded `Buffer` API usage and
your users will not observe a runtime deprecation warning when running your code on Node.js 10._

<a id="variant-2"></a>
## Variant 2: Use a polyfill
## <!--variant-2-->Variant 2: Use a polyfill

There are three different polyfills available:

Expand Down Expand Up @@ -136,8 +133,7 @@ is recommended.

_Don't forget to drop the polyfill usage once you drop support for Node.js < 4.5.0._

<a id="variant-3"></a>
## Variant 3 — Manual detection, with safeguards
## <!--variant-3-->Variant 3 — Manual detection, with safeguards

This is useful if you create `Buffer` instances in only a few places (e.g. one), or you have your own
wrapper around them.
Expand Down Expand Up @@ -223,11 +219,9 @@ leaking to the remote attacker.
_Note that the same applies to `new Buffer()` usage without zero-filling, depending on the Node.js
version (and lacking type checks also adds DoS to the list of potential problems)._

<a id="faq"></a>
## FAQ

<a id="design-flaws"></a>
### What is wrong with the `Buffer` constructor?
### <!--design-flaws-->What is wrong with the `Buffer` constructor?

The `Buffer` constructor could be used to create a buffer in many different ways:

Expand Down Expand Up @@ -280,8 +274,7 @@ When using `Buffer.from(req.body.string)` instead, passing a number will always
throw an exception instead, giving a controlled behavior that can always be
handled by the program.

<a id="ecosystem-usage"></a>
### The `Buffer()` constructor has been deprecated for a while. Is this really an issue?
### <!--ecosystem-usage-->The `Buffer()` constructor has been deprecated for a while. Is this really an issue?

Surveys of code in the `npm` ecosystem have shown that the `Buffer()` constructor is still
widely used. This includes new code, and overall usage of such code has actually been
Expand Down
10 changes: 5 additions & 5 deletions scripts/plugins/anchor-markdown-headings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
* `<!---->` to quote your English anchor name inside, with your
* own title beside it.
*/
module.exports = function anchorMarkdownHeadings (text, level, raw) {
module.exports = function anchorMarkdownHeadings (text, level, raw, slugger) {
// Check whether we've got the comment matches
// <!--comment-->, <!--comment --> and even <!-- comment-->
// (20 hex = 32 dec = space character)
Expand Down Expand Up @@ -42,8 +42,8 @@ module.exports = function anchorMarkdownHeadings (text, level, raw) {

anchorTitle = anchorTitle.toLowerCase()

return '<h' + level + ' id="header-' + anchorTitle + '">' + text + '<a name="' +
anchorTitle + '" class="anchor" href="#' +
anchorTitle + '" aria-labelledby="header-' +
anchorTitle + '"></a></h' + level + '>'
const anchorId = `${slugger ? slugger.slug(anchorTitle) : anchorTitle}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the marked docs I get the impression that the slugger argument will always be provided when our custom header renderer function (anchorMarkdownHeadings()) is called by marked. Or have I misunderstood?

If it is indeed always provided, could we invoke slugger.slug() directly, without having logic checking if slugger is indeed provided? Primarily to help new contributors and our future selfs when trying to understand what this code does later, not having to graps unnecessary logic is valuable.

const headerId = `header-${anchorId}`

return `<h${level} id="${headerId}">${text}<a id="${anchorId}" class="anchor" href="#${anchorId}" aria-labelledby="${headerId}"></a></h${level}>`
}
33 changes: 26 additions & 7 deletions tests/scripts/anchor-mardown-headings.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
'use strict'

const marked = require('marked')
const test = require('tape')
const anchorMarkdownHeadings = require('../../scripts/plugins/anchor-markdown-headings')

test('anchorMarkdownHeadings', (t) => {
t.plan(6)
t.plan(7)

t.test('correctly parses markdown heading without links', (t) => {
const text = 'Simple title'
const level = 1
const raw = 'Simple title'
const output = anchorMarkdownHeadings(text, level, raw)
const expected = '<h1 id="header-simple-title">Simple title' +
'<a name="simple-title" class="anchor" ' +
'href="#simple-title" aria-labelledby="header-simple-title"></a></h1>'
'<a id="simple-title" class="anchor" href="#simple-title" ' +
'aria-labelledby="header-simple-title"></a></h1>'

t.plan(1)
t.equal(output, expected)
Expand All @@ -26,7 +27,7 @@ test('anchorMarkdownHeadings', (t) => {
const output = anchorMarkdownHeadings(text, level, raw)
const expected = '<h3 id="header-title-with-link">Title with ' +
'<a href="#">link</a>' +
'<a name="title-with-link" class="anchor" href="#title-with-link" ' +
'<a id="title-with-link" class="anchor" href="#title-with-link" ' +
'aria-labelledby="header-title-with-link"></a></h3>'

t.plan(1)
Expand All @@ -40,7 +41,7 @@ test('anchorMarkdownHeadings', (t) => {
const output = anchorMarkdownHeadings(text, level, raw)
const expected = '<h2 id="header-a-b-cde">a <a href="b">b</a> c' +
'<a href="d">d</a>e' +
'<a name="a-b-cde" class="anchor" href="#a-b-cde" ' +
'<a id="a-b-cde" class="anchor" href="#a-b-cde" ' +
'aria-labelledby="header-a-b-cde"></a></h2>'

t.plan(1)
Expand All @@ -53,7 +54,7 @@ test('anchorMarkdownHeadings', (t) => {
const raw = '$$$ WIN BIG! $$$'
const output = anchorMarkdownHeadings(text, level, raw)
const expected = '<h4 id="header-win-big">$$$ WIN BIG! $$$' +
'<a name="win-big" class="anchor" href="#win-big" ' +
'<a id="win-big" class="anchor" href="#win-big" ' +
'aria-labelledby="header-win-big"></a></h4>'

t.plan(1)
Expand All @@ -67,7 +68,8 @@ test('anchorMarkdownHeadings', (t) => {
const output = anchorMarkdownHeadings(text, level, raw)
const expected = '<h2 id="header-anchor-with-non-english-characters">' +
'这是<a href="b">链接</a>的<a href="d">测试！</a>' +
'<a name="anchor-with-non-english-characters" class="anchor" href="#anchor-with-non-english-characters" ' +
'<a id="anchor-with-non-english-characters" class="anchor" ' +
'href="#anchor-with-non-english-characters" ' +
'aria-labelledby="header-anchor-with-non-english-characters"></a></h2>'

t.plan(1)
Expand All @@ -84,4 +86,21 @@ test('anchorMarkdownHeadings', (t) => {
t.plan(1)
t.equal(output, expected)
})

t.test('does not generate duplicate IDs', (t) => {
const renderer = new marked.Renderer()
renderer.heading = anchorMarkdownHeadings

const text = '# Title\n# Title'
const output = marked(text, { renderer: renderer })
const expected = '<h1 id="header-title">Title' +
'<a id="title" class="anchor" ' +
'href="#title" aria-labelledby="header-title"></a></h1>' +
'<h1 id="header-title-1">Title' +
'<a id="title-1" class="anchor" ' +
'href="#title-1" aria-labelledby="header-title-1"></a></h1>'

t.plan(1)
t.equal(output, expected)
})
})