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

tools: update marked to 0.4.0 #21081

Closed
wants to merge 7 commits into from
Closed

tools: update marked to 0.4.0 #21081

wants to merge 7 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 1, 2018

Update marked to 0.4.0 for use with the tools/doc/*.js tools.

Ref: https://nodesecurity.io/advisories/531

(Yes, that is extraordinarily unlikely to impact us in this context.)

@styfle @vsemozhetbyt

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott Trott requested a review from a team as a code owner June 1, 2018 11:29
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Jun 1, 2018
@Trott Trott removed the request for review from a team June 1, 2018 11:29
@@ -1,19 +0,0 @@
Copyright (c) 2011-2014, Christopher Jeffrey (https://github.com/chjj/)
Copy link
Member

@richardlau richardlau Jun 1, 2018

Choose a reason for hiding this comment

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

We'll have to fix the license-builder script:

addlicense "marked" "tools/doc/node_modules/marked" \
"$(cat ${rootdir}/tools/doc/node_modules/marked/LICENSE)"

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated license-builder.sh. Thanks.

@styfle
Copy link
Member

styfle commented Jun 1, 2018

@Trott Why is the node_modules folder checked in?
I wasn't expecting to see the source code of marked here 😆

@Trott
Copy link
Member Author

Trott commented Jun 1, 2018

@Trott Why is the node_modules folder checked in?

I'm not 100% sure, but I imagine the problem it is supposed to solve is probably solved by package-lock.json.

@vsemozhetbyt
Copy link
Contributor

@Trott I shall try to check if there are any breaking changes in our HTML/JSON docs with this update. Last time I've checked, there were some.

@styfle
Copy link
Member

styfle commented Jun 1, 2018

Is there a build step to generate all the HTML files?
I want to try this on my local machine but all I see is a generate.js asking for a filename?

@Trott
Copy link
Member Author

Trott commented Jun 1, 2018

@styfle make doc (and make test-doc too)

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 1, 2018

To build just any doc:

node tools/doc/generate.js --format=json doc/api/all.md > all.json
node tools/doc/generate.js --format=html --node-version=11.0.0 --analytics= doc/api/all.md > all.html

@@ -0,0 +1 @@
../marked/bin/marked
Copy link
Member

Choose a reason for hiding this comment

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

Can you do this to remove node modules?

cd tools/doc && echo 'node_modules' > .gitignore && rm -rf node_modules && git add -A

Copy link
Member Author

Choose a reason for hiding this comment

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

We have other tools directory node_modules subdirectories listed as ignored in the top-level .gitignore so putting it there is probably the way to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we'd need to add an npm install step for that directory in Makefile and vcbuild.bat similar to lint-md-build.

Copy link
Member

Choose a reason for hiding this comment

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

Is it going to cause a problem to run npm install each time in the Makefile?
I'm still new to the build tools 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

@nodejs/build ^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

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

@styfle @Trott Probably not a big issue, but would obviously be better to avoid doing so (would cause longer build times)

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not a big issue, but would obviously be better to avoid doing so (would cause longer build times)

I guess that might explain checking in the node_modules directory. :-D

@nodejs nodejs deleted a comment from refack Jun 1, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 2, 2018

I've started to check the diff in HTML and JSON output before and after this upgrade. There are many insignificant changes: white spaces (+/- line breaks), code blocks class names (class="lang- -> class="language-), list metadata, HTML entities vs. verbatim symbols etc.

But here is the first detected really breaking change:

Before:

1

After:

2

Cause: markdown links are not rendered as links inside HTML markup.

Small test to check:

'use strict';

const marked = require('marked');

const md = `
<div>A [link][].</div>

[link]: #hash
`;

const tokens = marked.lexer(md);
console.log(tokens);

const html = marked.parser(tokens);
console.log(`\n${html}`);

With the current version:

[ { type: 'html', pre: false, text: '<div>A [link][].</div>\n\n' },
  links: { link: { href: '#hash', title: undefined } } ]

<div>A <a href="#hash">link</a>.</div>

With the new version:

[ { type: 'html', pre: false, text: '<div>A [link][].</div>\n\n' },
  links: { link: { href: '#hash', title: undefined } } ]

<div>A [link][].</div>

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 2, 2018

This issue concerns not links only, but all Markdown markup inside HTML markup (code fragments in backticks are not rendered as code too, for example).

@vsemozhetbyt
Copy link
Contributor

Issue 2, more complicated.

Before (5 links with right texts and URLs):

1

After (3 links with wrong text and URLs + 2 code spans instead of links):

2

Cause: some combination of links with backticks (some of them are in full form like [text][ref], some of them are contracted like [text-as-ref]) baffles parser and produces very tangled output.

Reduced test case with the same pattern as in the source above:

'use strict';

const marked = require('marked');

const md = `
Links: [\`link1\`] and [\`link2\`] and [\`link3\`] and
[\`link 4\`][\`link4\`] and [\`link 4\`][\`link4\`], end.

[\`link1\`]: #hash1
[\`link2\`]: #hash2
[\`link3\`]: #hash3
[\`link4\`]: #hash4
`;

const tokens = marked.lexer(md);
console.log(tokens);

const html = marked.parser(tokens);
console.log(`\n${html}`);

With the current version:

[ { type: 'paragraph',
    text:
     'Links: [`link1`] and [`link2`] and [`link3`] and\n[`link 4`][`link4`] and [`link 4`][`link4`], end.' },
  links: { '`link1`': { href: '#hash1', title: undefined },
    '`link2`': { href: '#hash2', title: undefined },
    '`link3`': { href: '#hash3', title: undefined },
    '`link4`': { href: '#hash4', title: undefined } } ]

<p>Links: <a href="#hash1"><code>link1</code></a> and <a href="#hash2"><code>link2</code></a> and <a href="#hash3"><code>link3</code></a> and
<a href="#hash4"><code>link 4</code></a> and <a href="#hash4"><code>link 4</code></a>, end.</p>

With the new version:

[ { type: 'paragraph',
    text:
     'Links: [`link1`] and [`link2`] and [`link3`] and\n[`link 4`][`link4`] and [`link 4`][`link4`], end.' },
  { type: 'space' },
  links: { '`link1`': { href: '#hash1', title: undefined },
    '`link2`': { href: '#hash2', title: undefined },
    '`link3`': { href: '#hash3', title: undefined },
    '`link4`': { href: '#hash4', title: undefined } } ]

<p>Links: <a href="#hash4"><code>link1</code>] and <a href="#hash4"><code>link2</code>] and <a href="#hash3"><code>link3</code></a> and
[<code>link 4</code></a> and [<code>link 4</code></a>, end.</p>

@styfle
Copy link
Member

styfle commented Jun 2, 2018

Possibly related: markedjs/marked#1263

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 2, 2018

Issue 1.2. Most likely related to the first issue: text links inside HTML markup are not auto-linkified.

Before:

1

After:

2

Small test:

'use strict';

const marked = require('marked');

const md = `
<table><tr><td>
See https://example.org for more info.
</td></tr></table>
`;

const tokens = marked.lexer(md);
console.log(tokens);

const html = marked.parser(tokens);
console.log(`\n${html}`);

With the current version:

[ { type: 'html',
    pre: false,
    text:
     '<table><tr><td>\nSee https://example.org for more info.\n</td></tr></table>\n' },
  links: {} ]

<table><tr><td>
See <a href="https://example.org">https://example.org</a> for more info.
</td></tr></table>

With the new version:

[ { type: 'html',
    pre: false,
    text:
     '<table><tr><td>\nSee https://example.org for more info.\n</td></tr></table>\n' },
  links: {} ]

<table><tr><td>
See https://example.org for more info.
</td></tr></table>

@vsemozhetbyt
Copy link
Contributor

Issue 3. Period is wrongly included in URL of auto-linkified text link inside parentheses.

Before:

1

After:

2

Small test:

'use strict';

const marked = require('marked');

const md = `
(See https://github.com/nodejs/node/pull/12562.)
`;

const tokens = marked.lexer(md);
console.log(tokens);

const html = marked.parser(tokens);
console.log(`\n${html}`);

With the current version:

[ { type: 'paragraph',
    text: '(See https://github.com/nodejs/node/pull/12562.)' },
  links: {} ]

<p>(See <a href="https://github.com/nodejs/node/pull/12562">https://github.com/nodejs/node/pull/12562</a>.)</p>

With the new version:

[ { type: 'paragraph',
    text: '(See https://github.com/nodejs/node/pull/12562.)' },
  links: {} ]

<p>(See <a href="https://github.com/nodejs/node/pull/12562.">https://github.com/nodejs/node/pull/12562.</a>)</p>

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 2, 2018

Issue 4. Markdown tables are not rendered.

Before:

1

After:

2

Small test:

'use strict';

const marked = require('marked');

const md = `

| a | b |
|-------|
| c | d |

`;

const tokens = marked.lexer(md);
console.log(tokens);

const html = marked.parser(tokens);
console.log(`\n${html}`);

With the current version:

[ { type: 'space' },
  { type: 'table',
    header: [ 'a', 'b' ],
    align: [ null ],
    cells: [ [Array] ] },
  links: {} ]

<table>
<thead>
<tr>
<th>a</th>
<th>b</th>
</tr>
</thead>
<tbody>
<tr>
<td>c</td>
<td>d</td>
</tr>
</tbody>
</table>

With the new version:

[ { type: 'space' },
  { type: 'paragraph', text: '| a | b |\n|-------|\n| c | d |' },
  { type: 'space' },
  links: {} ]

<p>| a | b |
|-------|
| c | d |</p>

@vsemozhetbyt
Copy link
Contributor

Issue 5. Tricky one.

Before:

1

After:

2

Cause: Links with bottom references that clash with Object.prototype properties are ignored by the lexer and are not rendered.

Small test:

'use strict';

const marked = require('marked');

const md = `

Link: [constructor][].

[constructor]: https://example.org/

`;

const tokens = marked.lexer(md);
console.log(tokens);

const html = marked.parser(tokens);
console.log(`\n${html}`);

With the current version:

[ { type: 'space' },
  { type: 'paragraph', text: 'Link: [constructor][].' },
  links: { constructor: { href: 'https://example.org/', title: undefined } } ]

<p>Link: <a href="https://example.org/">constructor</a>.</p>

With the new version:

[ { type: 'space' },
  { type: 'paragraph', text: 'Link: [constructor][].' },
  { type: 'space' },
  links: {} ]

<p>Link: [constructor][].</p>

@vsemozhetbyt
Copy link
Contributor

It seems this is all I could find. The new version also resolves conflicting references (see #20100) in a different way, but this is our mess and we can ignore this for now.

However, we may need to wait for fixing above-enumerated issues before the upgrade.

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Jun 3, 2018
@Trott
Copy link
Member Author

Trott commented Jun 3, 2018

Thanks for finding and clearly detailing all the issues that need to be addressed, @vsemozhetbyt! Labeling in progress for now.

@Trott Trott force-pushed the marked-0.4.0 branch 2 times, most recently from 514bea0 to 09cdca8 Compare June 19, 2018 20:10
@Trott
Copy link
Member Author

Trott commented Jun 19, 2018

markdown links are not rendered as links inside HTML markup.

@vsemozhetbyt That appears to be a bug in our markdown and not a bug in the marked library.

https://daringfireball.net/projects/markdown/syntax#html:

Note that Markdown formatting syntax is not processed within block-level HTML tags.

@Trott
Copy link
Member Author

Trott commented Jun 19, 2018

@vsemozhetbyt That appears to be a bug in our markdown and not a bug in the marked library.

Solution is probably to have html.js process the markdown before adding the div tag. I'll see about doing that as part of this PR.

@Trott
Copy link
Member Author

Trott commented Jun 20, 2018

Solution is probably to have html.js process the markdown before adding the div tag. I'll see about doing that as part of this PR.

Well, that was a bit long on the debugging front, but it turns out the solution was to remove a single unnecessary line from html.js. That resolved issue 1 identified by @vsemozhetbyt (with the amazingly helpful screenshots--thanks so much for all the work identifying the problems and providing excellent bug reports in the form of those comments with images!). Now to hope the other issues can be resolved more promptly...

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 30, 2018

  1. We still have to do something with markdown inside HTML.

The source in crypto.md:

  <tr>
    <td><code>RSA_PSS_SALTLEN_DIGEST</code></td>
    <td>Sets the salt length for `RSA_PKCS1_PSS_PADDING` to the digest size
        when signing or verifying.</td>
  </tr>
  <tr>
    <td><code>RSA_PSS_SALTLEN_MAX_SIGN</code></td>
    <td>Sets the salt length for `RSA_PKCS1_PSS_PADDING` to the maximum
        permissible value when signing data.</td>
  </tr>
  <tr>
    <td><code>RSA_PSS_SALTLEN_AUTO</code></td>
    <td>Causes the salt length for `RSA_PKCS1_PSS_PADDING` to be determined
        automatically when verifying a signature.</td>
  </tr>

Before:

  <tr>
    <td><code>RSA_PSS_SALTLEN_DIGEST</code></td>
    <td>Sets the salt length for <code>RSA_PKCS1_PSS_PADDING</code> to the digest size
        when signing or verifying.</td>
  </tr>
  <tr>
    <td><code>RSA_PSS_SALTLEN_MAX_SIGN</code></td>
    <td>Sets the salt length for <code>RSA_PKCS1_PSS_PADDING</code> to the maximum
        permissible value when signing data.</td>
  </tr>
  <tr>
    <td><code>RSA_PSS_SALTLEN_AUTO</code></td>
    <td>Causes the salt length for <code>RSA_PKCS1_PSS_PADDING</code> to be determined
        automatically when verifying a signature.</td>
  </tr>

After (see preserved backticks instead of <code> elements):

  <tr>
    <td><code>RSA_PSS_SALTLEN_DIGEST</code></td>
    <td>Sets the salt length for `RSA_PKCS1_PSS_PADDING` to the digest size
        when signing or verifying.</td>
  </tr>
  <tr>
    <td><code>RSA_PSS_SALTLEN_MAX_SIGN</code></td>
    <td>Sets the salt length for `RSA_PKCS1_PSS_PADDING` to the maximum
        permissible value when signing data.</td>
  </tr>
  <tr>
    <td><code>RSA_PSS_SALTLEN_AUTO</code></td>
    <td>Causes the salt length for `RSA_PKCS1_PSS_PADDING` to be determined
        automatically when verifying a signature.</td>
  </tr>

Also in fs.md:

    <td>Flag indicating reading accesses to the file system will no longer
    result in an update to the `atime` information associated with the file.
    This flag is available on Linux operating systems only.</td>

Also several fragments in os.md tables.

Also stream.md produces unrendered links in tables for the same reason.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 30, 2018

Another example of issue 1:
errors.md:

For *all* [`EventEmitter`][] objects, if an `'error'` event handler is not
provided, the error will be thrown, causing the Node.js process to report an
uncaught exception and crash unless either: The [`domain`][domains] module is
used appropriately or a handler has been registered for the
[`'uncaughtException'`][] event.

Before:

<p>For <em>all</em> <a href="events.html#events_class_eventemitter"><code>EventEmitter</code></a> objects, if an <code>&#39;error&#39;</code> event handler is not
provided, the error will be thrown, causing the Node.js process to report an
uncaught exception and crash unless either: The <a href="domain.html"><code>domain</code></a> module is
used appropriately or a handler has been registered for the
<a href="process.html#process_event_uncaughtexception"><code>&#39;uncaughtException&#39;</code></a> event.</p>

After:

<p>For <em>all</em> <a href="domain.html"><code>EventEmitter</code>][] objects, if an <code>&#39;error&#39;</code> event handler is not
provided, the error will be thrown, causing the Node.js process to report an
uncaught exception and crash unless either: The [<code>domain</code></a> module is
used appropriately or a handler has been registered for the
<a href="process.html#process_event_uncaughtexception"><code>&#39;uncaughtException&#39;</code></a> event.</p>

Also in net.md, these fragments produce similar mess:

Source:
### net.createConnection(path[, connectListener])
<!-- YAML
added: v0.1.90
-->

* `path` {string} Path the socket should connect to. Will be passed to
  [`socket.connect(path[, connectListener])`][`socket.connect(path)`].
  See [Identifying paths for IPC connections][].
* `connectListener` {Function} Common parameter of the
  [`net.createConnection()`][] functions, an "once" listener for the
  `'connect'` event on the initiating socket. Will be passed to
  [`socket.connect(path[, connectListener])`][`socket.connect(path)`].
* Returns: {net.Socket} The newly created socket used to start the connection.

Initiates an [IPC][] connection.

This function creates a new [`net.Socket`][] with all options set to default,
immediately initiates connection with
[`socket.connect(path[, connectListener])`][`socket.connect(path)`],
then returns the `net.Socket` that starts the connection.

### net.createConnection(port[, host][, connectListener])
<!-- YAML
added: v0.1.90
-->

* `port` {number} Port the socket should connect to. Will be passed to
  [`socket.connect(port[, host][, connectListener])`][`socket.connect(port, host)`].
* `host` {string} Host the socket should connect to. Will be passed to
  [`socket.connect(port[, host][, connectListener])`][`socket.connect(port, host)`].
   **Default:** `'localhost'`.
* `connectListener` {Function} Common parameter of the
  [`net.createConnection()`][] functions, an "once" listener for the
  `'connect'` event on the initiating socket. Will be passed to
  [`socket.connect(path[, connectListener])`][`socket.connect(port, host)`].
* Returns: {net.Socket} The newly created socket used to start the connection.

Initiates a TCP connection.

This function creates a new [`net.Socket`][] with all options set to default,
immediately initiates connection with
[`socket.connect(port[, host][, connectListener])`][`socket.connect(port, host)`],
then returns the `net.Socket` that starts the connection.

Also many problem fragments in stream.md like these:

The `writable.cork()` method forces all written data to be buffered in memory.
The buffered data will be flushed when either the [`stream.uncork()`][] or
[`stream.end()`][stream-end] methods are called.
For backwards compatibility reasons, removing [`'data'`][] event handlers will
**not** automatically pause the stream. Also, if there are piped destinations,
then calling [`stream.pause()`][stream-pause] will not guarantee that the
stream will *remain* paused once those destinations drain and ask for more data.

@vsemozhetbyt
Copy link
Contributor

I've updated the last comments with more examples.

Sum up: on this stage, we still have 2 issues with the new marked version: 1) some pattern with several consecutive markdown links produses a mess and 2) markdown code and link fragments are not processed inside HTML tables.

Trott added 7 commits July 1, 2018 19:40
Remove unneeded line that will cause links to break when we update to
marked 0.4.0.
Simplify the inline markdown in buffer.md. Admission: This is to work
around an issue that arises when this markdown is processed by `marked`
v0.4.0.
Links in crypto.md table had previously been autolinking, but correct
Mardkwon processing will not autolink in block elements like tables.
Upcoming marked 0.4.0 will not autolink in this instance, so add the
links as anchor elements.
Update marked to 0.4.0 for use with the tools/doc/*.js tools.
Float a patch to deal with punctuation at the end of URLs in a more
robust fashion.

Refs: markedjs/marked#1293
Float a patch to deal with empty table cells.

Refs: markedjs/marked#1262
Float a patch on marked-0.4.0 that fixes a bug that breaks links that
are named the same as properties on the Object prototype.

Refs: markedjs/marked#1299
@Trott
Copy link
Member Author

Trott commented Jul 2, 2018

Issue 1 is a bug in marked that is non-trivial to fix without breaking other things, but I'll try.

Issue 2 is a bug in our markdown and I wouldn't be surprised if other markdown processors also had the same issue. For those, I'll try to fix our markdown.

@AyushG3112
Copy link
Contributor

AyushG3112 commented Jul 2, 2018

In case we float any patches, would it be viable to note them as things to take care of when the nodejs website is redesigned, which is being thought about, according to this issue: nodejs/nodejs.org#1534 ?

@rubys
Copy link
Member

rubys commented Jul 2, 2018

@Trott I'm curious as to the bug in our markdown? The following works with both marked and remark:

'use strict';

const marked = require('marked');

const unified = require('unified');
const markdown = require('remark-parse');
const remark2rehype = require('remark-rehype');
const html = require('rehype-stringify');
const raw = require('rehype-raw');

const testData = `
|   h   |  h  |     h     |
|-------|-----|-----------|
|<b>x<b>|**x**|[x](a.html)|
`

console.log('marked:');
console.log(marked(testData));

console.log('');

console.log('unified/remark/rehype:');
console.log(unified()
  .use(markdown)
  .use(remark2rehype, {allowDangerousHTML: true})
  .use(raw)
  .use(html)
  .processSync(testData).toString().trim());

@Trott
Copy link
Member Author

Trott commented Jul 2, 2018

@rubys Yes, that example seems to work correctly in both. Markdown inside a markdown table is 👍.

This code should show the issue. Markdown inside HTML block elements should not be expanded:

<table>
<tr>
<td>**should not be bold**</td>
</tr>
</table>

Some of our docs have markdown inside HTML table elements. According to markdown specs as I understand them, that markdown should not be expanded. It was expanded in marked 0.3.5 and that's a bug that's fixed in marked 0.4.0. Unfortunately, we're relying on that bug.

@rubys
Copy link
Member

rubys commented Jul 2, 2018

@Trott gotcha. I verified that that marked 0.4 and remark produce the same output for that input. Note: given how unified pipelining works, it would not be difficult to write a processor that looks for text nodes within a table and expand them. It could even look for tables with a given data-somethingorother attribute defined, and only process those tables. This could also be done with marked by looking at the lexed tokens.

@rubys
Copy link
Member

rubys commented Jul 9, 2018

Question: what is the status of this effort, and on evaluating the possible conversion to remark? I ask because it may impact other work.

At the moment, it seems clear that remark is easier to extend cleanly, but runs slower. Moving the generation of all.html and all.json away from reprocessing the input (done) and generating both the html and json in one pass (planned) will mitigate the performance difference. The generation of JSON isn't complete, but a proof of concept has been posted.

marked also seems less "spec compliant" and more prone to change from release to release, but that might be because we don't have enough experience with remark.

Additionally, the build process for node already is using remark... it would be ideal to consolidate to one markdown engine.

Am I missing anything? (And, yes, at this point I may be biased :-) )

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 10, 2018

As for the new toolchain with remark, one of the possible concerns to be considered is the learning curve. We have a very small collaborators group that is aware of doc tools infrastructure. The current tooling was rather old and tangled and there still was not so big interest in changing it. I am just afraid if we will have enough collaborators to maintain the new tooling in an effective, responsive and updatable state. It may be not good to have a too small doc tool collaborators team more like a closed esoteric sect.

@Trott
Copy link
Member Author

Trott commented Jul 10, 2018

I think the effort of @rubys to get remark in place is exceedingly likely to be completed before this is. And I think if that happens, we'll at least have @rubys as someone who understands the tooling. :-D

I'm leaving this open because I will indeed push it forward if something happens to the remark effort that stalls it. But I don't foresee that.

I'm also leaving it open as a to-do list of fixing the table markup in places where it is not compliant.

I would encourage moving ahead with remark. But @vsemozhetbyt (and possibly others) may have legitimate reservations that need to be considered.

@rubys
Copy link
Member

rubys commented Jul 12, 2018

I was originally going to say something rather meek like "as someone new to both marked and remark, I find remark more approachable", but having just completed my first pass at updating the conversion to JSON, it is clear that current marked based code to produce JSON is essentially unmaintainable. I've surfaced a number of issues in #21697, but the full set of differences can be found by looking for continue and return statements in jsondiff. While it is possible that a few of these are misunderstandings on my part, the bulk are clearly wrong.

As a byproduct of this effort, I compared the output of the conversions for every description, and will submit a separate pull request with changes to the markdown to make it work with both sets of toolchains.

@Trott
Copy link
Member Author

Trott commented Jul 12, 2018

I'm feeling good about the progress @rubys has been making moving to remark and #21780 deals with all the quirks that I was going to get around to sooner or later, so I'm inclined to close this at this point. We can always re-open it if the move to remark stalls or hits an unforeseen insurmountable problem.

@Trott Trott closed this Jul 12, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 12, 2018

The problem with doctools is that they are rather old and it seems we may have no active collaborators that created them. I've done some cleanup in doctools this spring and json.js was the most complicated one, sometimes I felt that I went in darkness or minefield) Moreover, I had no context about the use cases for JSON results or any criteria for required completeness and validness of the results. So if anybody wants to even completely recreate the json tools contacting with JSON users, this may be the best, if they can maintain the new scripts to some extent.

@rubys
Copy link
Member

rubys commented Jul 12, 2018

If we can make progress landing the changes I've made to convert from marked to remark, I'll certainly support the code. If you are looking for somebody to own the current marked based implementation of JSON generation... sorry, but I would need to throw a ENOTME Error on that. :-)

@vsemozhetbyt
Copy link
Contributor

Any tool will do with a human being helping, I think)

@tolmasky
Copy link
Contributor

tolmasky commented Jul 12, 2018 via email

@vsemozhetbyt
Copy link
Contributor

We state JSON output as experimental now, so it seems we have any wiggle room we want.

@Trott Trott deleted the marked-0.4.0 branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants