★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21697Nouvelle recherche | Portail wanayoo
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

Projects
None yet
7 participants
@rubys
Copy link
Member

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
@rubys

This comment has been minimized.

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.

This comment has been minimized.

@jasnell

jasnell Jul 10, 2018

Member

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.

This comment has been minimized.

@rubys

rubys Jul 10, 2018

Author Member

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.

This comment has been minimized.

@jasnell

jasnell Jul 10, 2018

Member

Ok, thanks for clarification :-)

@rubys

This comment has been minimized.

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 referenced this pull request Jul 12, 2018

Closed

tools: update marked to 0.4.0 #21081

2 of 2 tasks complete
@rubys

This comment has been minimized.

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

This comment has been minimized.

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.

@rubys rubys referenced this pull request Jul 20, 2018

Closed

Produce HTML documentation using unified/remark/rehype #21490

3 of 3 tasks complete

@Trott Trott removed the blocked label Jul 20, 2018

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jul 20, 2018

Removed the blocked label.

@rubys rubys force-pushed the rubys:remark-json branch from edb8cc6 to b80719b Jul 21, 2018

@vsemozhetbyt

This comment was marked as outdated.

@vsemozhetbyt

This comment was marked as outdated.

@vsemozhetbyt

This comment has been minimized.

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jul 22, 2018

CI is all green.

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jul 22, 2018

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.

@vsemozhetbyt
Copy link
Member

vsemozhetbyt left a comment

Just some comment nits.

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

// extract definitions

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

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}

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

Nit: output: { name: module }.

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

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

Nit: it seems this can be unwrapped.


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

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

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

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

Ditto in description. If.

` > ${text}`
);

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

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

Ditto in match. If.

}
return;
}
// Stablility marker: > Stability: ...

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

Stablility -> Stability

return src;
}


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

// To reduse escape slashes in RegExp string components.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

Sorry, old debt: reduse -> reduce.

return src;
}


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

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Jul 22, 2018

Member

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

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jul 22, 2018

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

This comment has been minimized.

@Trott

Trott approved these changes Jul 23, 2018

Copy link
Member

Trott left a comment

Rubber-stamp LGTM

@trivikr
Copy link
Contributor

trivikr left a comment

RSLGTM

@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jul 24, 2018

@rubys This need rebasing (probably after b38b8d3).

rubys added some commits Jul 21, 2018

premature to remove marked entirely
... but we can update to the latest :-)

@rubys rubys force-pushed the rubys:remark-json branch from a5ee58c to 93d2044 Jul 24, 2018

@rubys

This comment has been minimized.

Copy link
Member Author

rubys commented Jul 24, 2018

rebased

@vsemozhetbyt

This comment has been minimized.

vsemozhetbyt added a commit that referenced this pull request Jul 25, 2018

tools: produce JSON documentation using unified/remark/rehype
PR-URL: #21697
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@vsemozhetbyt

This comment has been minimized.

Copy link
Member

vsemozhetbyt commented Jul 25, 2018

Landed in f41dd55
Thank you!

@targos

This comment has been minimized.

Copy link
Member

targos commented Jul 26, 2018

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

@targos targos referenced this pull request Jul 31, 2018

Closed

tools: convert addon-verify to remark #21978

2 of 2 tasks complete

targos added a commit that referenced this pull request Aug 7, 2018

tools: produce JSON documentation using unified/remark/rehype
PR-URL: #21697
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

tniessen added a commit to tniessen/node that referenced this pull request Sep 7, 2018

@tniessen tniessen referenced this pull request Sep 7, 2018

Closed

build: make doc generation work on Windows #22749

2 of 2 tasks complete

tniessen added a commit to tniessen/node that referenced this pull request Sep 8, 2018

build: make doc generation work on Windows
PR-URL: nodejs#22749
Refs: nodejs#21697
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Sep 9, 2018

build: make doc generation work on Windows
PR-URL: #22749
Refs: #21697
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

build: make doc generation work on Windows
PR-URL: nodejs#22749
Refs: nodejs#21697
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.