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

Generated JSON documentation no longer includes methods field. #31290

Closed
Me1000 opened this issue Jan 9, 2020 · 6 comments · Fixed by #31294
Closed

Generated JSON documentation no longer includes methods field. #31290

Me1000 opened this issue Jan 9, 2020 · 6 comments · Fixed by #31294

Comments

@Me1000
Copy link

Me1000 commented Jan 9, 2020

  • Version: 13.6.0

PR #31086 Changed the markdown styling for documentation. This introduced a bug in the JSON documentation which gets generated with the release. Specifically the methods field is no longer there:

e.g. in 13.5.0:

  source: 'node/doc/api/assert.md',
  modules: [
    {
      textRaw: 'Assert',
      name: 'assert',
      introduced_in: 'v0.1.21',
      stability: 2,
      stabilityText: 'Stable',
      desc: '<p>The <code>assert</code> module provides a set of assertion functions for verifying\n' +
        'invariants. The module provides a recommended <code>strict</code> mode and a more\n' +
        'lenient legacy mode.</p>\n',
      classes: [Array],
      modules: [Array],
      methods: [Array],
      type: 'module',
      displayName: 'Assert'
    }
  ]
}

In 13.6.0 ▶️:

  source: 'node/doc/api/assert.md',
  modules: [
    {
      textRaw: 'Assert',
      name: 'assert',
      introduced_in: 'v0.1.21',
      stability: 2,
      stabilityText: 'Stable',
      desc: '<p>The <code>assert</code> module provides a set of assertion functions for verifying\n' +
        'invariants. The module provides a recommended <code>strict</code> mode and a more\n' +
        'lenient legacy mode.</p>\n',
      classes: [Array],
      modules: [Array],
      type: 'module',
      displayName: 'Assert'
    }
  ]
}

It looks like this regex no longer matched: https://github.com/nodejs/node/blob/master/tools/doc/json.js#L475

Since they're really only used for the HTML styling, I think a simple fix might be to just strip the tick marks here:

const text = textJoin(header.children, file);

- const text = textJoin(header.children, file);
+ const text = textJoin(header.children, file).replace(/`/g, "");

I can submit a PR if others agree with this simple solution.

EDIT: Unfortunately that simple solution will not work. There were some existing headings with tick marks that parse incorrectly if they're removed. Continuing to investigate, but I'm still not super familiar with this code.

@Trott
Copy link
Member

Trott commented Jan 10, 2020

Yikes. Here's the relevant computed regexp: /^(?:(?:(?:(?:\\?_)+|\b)\w+\b|\\?\[[\w\.]+\\?\])\.?)*((?:(?:(?:\\?_)+|\b)\w+\b|\\?\[[\w\.]+\\?\]))\([^)]*\)$/ Fun! Something in there needs to accommodate the backtick character.

@Trott
Copy link
Member

Trott commented Jan 10, 2020

@Me1000 Can you check if this patch fixes the problem for you? It seems to fix it for me.

diff --git a/tools/doc/json.js b/tools/doc/json.js
index e07486265c..9ec6394085 100644
--- a/tools/doc/json.js
+++ b/tools/doc/json.js
@@ -442,6 +442,8 @@ const maybeClassPropertyPrefix = '(?:Class Property: +)?';
 const maybeQuote = '[\'"]?';
 const notQuotes = '[^\'"]+';
 
+const maybeBacktick = '[`]?';
+
 // To include constructs like `readable\[Symbol.asyncIterator\]()`
 // or `readable.\_read(size)` (with Markdown escapes).
 const simpleId = r`(?:(?:\\?_)+|\b)\w+\b`;
@@ -472,7 +474,8 @@ const headingExpressions = [
     `${classMethodPrefix}${maybeAncestors}(${id})${callWithParams}$`, 'i') },
 
   { type: 'method', re: RegExp(
-    `^${maybeAncestors}(${id})${callWithParams}$`, 'i') },
+    // eslint-disable-next-line max-len
+    `^${maybeBacktick}${maybeAncestors}(${id})${callWithParams}${maybeBacktick}$`, 'i') },
 
   { type: 'property', re: RegExp(
     `^${maybeClassPropertyPrefix}${ancestors}(${id})${noCallOrProp}$`, 'i') },

Also, I don't suppose you've checked to see if any of the other elements (class, ctor, classMethod, event, property) have missing entries too?

@Trott
Copy link
Member

Trott commented Jan 10, 2020

#31294 should fix this problem for methods. Further research is probably necessary to see if this needs to be done for the other regular expressions ((class, ctor, classMethod, event, property) or not.

@Trott
Copy link
Member

Trott commented Jan 10, 2020

+const maybeBacktick = '[`]?';

Heh, I guess that could just be:

+const maybeBacktick = '`?';

@Trott
Copy link
Member

Trott commented Jan 10, 2020

Looks like this bug effects events too.

@Trott
Copy link
Member

Trott commented Jan 10, 2020

Class names too. Probably all of them. I'll add 'em to the PR.

@Trott Trott closed this as completed in be46a72 Jan 12, 2020
MylesBorins pushed a commit that referenced this issue Jan 16, 2020
Methods, events, and so on in headers in our documentation may (and
should) be set off with backticks in the raw markdown. When that
happens, the headers is misinterpreted by tools/json.js as not being a
method or event. Update the JSON tool generator to accommodate backticks
in this situation and add a test for this situation.

Fixes: #31290

PR-URL: #31294
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
codebytere pushed a commit that referenced this issue Mar 14, 2020
Methods, events, and so on in headers in our documentation may (and
should) be set off with backticks in the raw markdown. When that
happens, the headers is misinterpreted by tools/json.js as not being a
method or event. Update the JSON tool generator to accommodate backticks
in this situation and add a test for this situation.

Fixes: #31290

PR-URL: #31294
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
codebytere pushed a commit that referenced this issue Mar 17, 2020
Methods, events, and so on in headers in our documentation may (and
should) be set off with backticks in the raw markdown. When that
happens, the headers is misinterpreted by tools/json.js as not being a
method or event. Update the JSON tool generator to accommodate backticks
in this situation and add a test for this situation.

Fixes: #31290

PR-URL: #31294
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 a pull request may close this issue.

2 participants