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: produce JSON documentation using unified/remark/rehype #21697

Closed
wants to merge 13 commits into from

Conversation

rubys
Copy link
Member

@rubys rubys commented Jul 7, 2018

work in progress. tools/doc/json2.js is a first pass attempt to convert tools/doc/json.js to a new markdown toolchain. The original intent was to produce functionally identical JSON, but some oddities have been found along the way. Examples: (highest priority first):

  • No events have parms in JSON with the original code. The new code picks up the parms.
  • The JSON produced for fs as a number with a method signature from the next heading.
  • Various textRaw and default properties seemingly randomly have an extra trailing space in their values.

The current (soon to be previous?) processing is based on a stream of tokens and a state machine. The new process is based on a pipeline of processors that have access to a tree of tokens. Care has been taken to not modify the stream so that this processor can be added to the pipeline that produces HTML.

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

@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 Jul 7, 2018
@Trott Trott added the wip Issues and PRs that are still a work in progress. label Jul 7, 2018
@rubys
Copy link
Member Author

rubys commented Jul 9, 2018

This pull request will require a lot of discussion. Other puzzles:

  • Why do methods have an inconsistent number of redundant and incomplete (only names and optional indicator) signatures? Examples:

    • asyncResource.emitDestroy has 0.
    • asyncResource.asyncId has 1.
    • buf.readDoubleBE has 2.
  • Where did all of the constructors go?

$ grep -l '"ctors"' out/doc/api/*.json
out/doc/api/all.json
out/doc/api/crypto.json
out/doc/api/stream.json
$ git grep -l '## Constructor:' doc/api/*.md
doc/api/inspector.md
doc/api/stream.md
doc/api/url.md
doc/api/vm.md
$ git grep -l '## new ' doc/api/*.md
doc/api/assert.md
doc/api/async_hooks.md
doc/api/buffer.md
doc/api/console.md
doc/api/crypto.md
doc/api/errors.md
doc/api/http.md
doc/api/net.md
doc/api/perf_hooks.md
doc/api/stream.md
doc/api/string_decoder.md
doc/api/tls.md
doc/api/util.md
doc/api/v8.md
doc/api/vm.md
doc/api/worker_threads.md

My current code is rejecting the constructor for console as the first signature defines parameters that are included in the list list below (at least not as first level items):

### new Console(stdout[, stderr][, ignoreErrors])
### new Console(options)
<!-- YAML
...
-->

* `options` {Object}
  * `stdout` {stream.Writable}
  * `stderr` {stream.Writable}
  * `ignoreErrors` {boolean} Ignore errors when writing to the underlying
    streams. **Default:** `true`.
  * `colorMode` {boolean|string} Set color support for this `Console` instance.
    Setting to `true` enables coloring while inspecting values, setting to
    `'auto'` will make color support depend on the value of the `isTTY` property
    and the value returned by `getColorDepth()` on the respective stream.
    **Default:** `'auto'`.

I can fix this up, but not seeing the Console constructor in the current output means that I'm flying blind.

@@ -0,0 +1,491 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Member

Choose a reason for hiding this comment

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

Is this a brand new file or is it pulled from another source? brand new files do not require the copyright header to be added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ultimately, it will be renamed to tools/doc/json2.js, and contains significant content from the original source.

Currently separate so that side by side comparisons of the output can be produced, via jsondiff.js, which will be removed when this effort is complete.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for clarification :-)

@rubys
Copy link
Member Author

rubys commented Jul 11, 2018

Latest issue: files like buffer.json sometimes parses stability correctly, e.g.:

"stability": 2

And other times misses it entirely:

"desc": "<p>Stability: 0 - Deprecated: Use\n..."

The issue here is the embedded newline. The bug here is that stabilityExpr needs dotAll or equivalent.

@rubys rubys mentioned this pull request Jul 12, 2018
2 tasks
@rubys
Copy link
Member Author

rubys commented Jul 12, 2018

This pull request is no longer in progress, nor is it ready to land. It depends on #21782 and #21780. In addition to providing fixes to the bugs mentioned above and listed in jsondiff.js, it differs from the output currently produced in that backslashes won't show up in names (for an example, look for "name": "\_\_filename" in modules.json.

Work remaining to be done before this lands: rename json2.js to json.js (overwriting the original source) and deleting jsondiff.js. For the moment, I'm leaving both sources and the diff tool with the thought that it would help reviewers.

Once #21490 lands, further refactoring and cleanup is possible.
html.js and json.js can be changed to export unified plugins and generate.js can implement the main pipeline that produces both. This will make it easy for other tools to be added to the pipeline, for example, see #21723. Also at that time, remark can be removed from package.json and the node_modules directory could be cleaned up.

@Trott Trott added blocked PRs that are blocked by other issues or PRs. and removed wip Issues and PRs that are still a work in progress. labels Jul 12, 2018
@Trott
Copy link
Member

Trott commented Jul 12, 2018

This pull request is no longer in progress, nor is it ready to land. It depends on #21782 and #21780.

Removed in progress and applied blocked.

@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Jul 20, 2018
@Trott
Copy link
Member

Trott commented Jul 20, 2018

Removed the blocked label.

@vsemozhetbyt

This comment has been minimized.

@vsemozhetbyt

This comment has been minimized.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

CI is all green.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 22, 2018

I've compared the diff in *.html results. It seems we have only this significant difference: some comments disappear in the new HTML docs, such as:

<!--introduced_in=v...-->

<!-- type=misc -->
<!--type=class-->
<!--type=module-->
<!-- type=global -->
<!-- type=var -->
<!--type=event-->
<!--type=example-->

<!-- name=dgram -->
<!--name=esm-->
<!--name=fs-->
<!--name=module-->
<!--name=SIGINT, SIGHUP, etc.-->
<!--name=querystring-->
<!--name=vm-->

Not sure if this is breaking in any aspect. Maybe we can ignore this change as these comments seem needed at the generation stage only.

@vsemozhetbyt
Copy link
Contributor

Ignorable note: json.js uses many iterations over elements using their properties with repetitive retrieving. Maybe it is worth to cache the properties once, especially if the element itself is not used. Say:

      nodes.forEach((node, i) => {

->

      nodes.forEach(({ type, value, children }, i) => {

or:

  return nodes.map((node) => {

->

  return nodes.map(({ type, position, value, children }) => {

etc.
Maybe this will speed up the process a little.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Just some comment nits.

const metaExpr = /<!--([^=]+)=([^-]+)-->\n*/g;
const stabilityExpr = /^Stability: ([0-5])(?:\s*-\s*)?(.*)$/s;

// extract definitions
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: // Extract definitions.

// Extract (and remove) metadata that is not directly inferable
// from the markdown itself.
nodes.forEach((node, i) => {
// Input: <!-- name=module -->; output: {name: module}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: output: { name: module }.

values.push(item);
current = item;
// Pluralize type to determine which 'bucket' to put this section
// in.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it seems this can be unwrapped.


delete section.list;
// Add this section to the parent. Sometimes we have two headings with a
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: extra space in parent. Sometimes.


delete section.list;
// Add this section to the parent. Sometimes we have two headings with a
// single blob of description. If the preceding entry at this level
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto in description. If.

` > ${text}`
);

// At this point, the name should match. If it doesn't find one that does.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto in match. If.

}
return;
}
// Stablility marker: > Stability: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Stablility -> Stability

return src;
}


// This section parse out the contents of an H# tag.

// To reduse escape slashes in RegExp string components.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, old debt: reduse -> reduce.

return src;
}


// This section parse out the contents of an H# tag.
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, old debt: parse out -> parses out.

@vsemozhetbyt
Copy link
Contributor

CC @nodejs/documentation @nodejs/build @nodejs/build-files: please, let us know if you plan to review this PR in the near future. Otherwise, I will land it, say, on Wednesday.

@vsemozhetbyt
Copy link
Contributor

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM

Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

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

RSLGTM

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jul 24, 2018

@rubys This need rebasing (probably after b38b8d3).

@rubys
Copy link
Member Author

rubys commented Jul 24, 2018

rebased

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt pushed a commit that referenced this pull request Jul 25, 2018
PR-URL: #21697
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Landed in f41dd55
Thank you!

@targos
Copy link
Member

targos commented Jul 26, 2018

Depends on #21616 to land on v10.x-staging

targos pushed a commit that referenced this pull request Aug 7, 2018
PR-URL: #21697
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
tniessen added a commit to tniessen/node that referenced this pull request Sep 7, 2018
tniessen added a commit to tniessen/node that referenced this pull request Sep 8, 2018
PR-URL: nodejs#22749
Refs: nodejs#21697
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Sep 9, 2018
PR-URL: #22749
Refs: #21697
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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.

7 participants