Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
Produce HTML documentation using unified/remark/rehype#21490
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
rubys commented Jun 23, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
rubys commented Jun 23, 2018
Replacing the remainder of calls to
The coding parts I'm comfortable with. It is the process by which code gets merged into the baseline that I'm interested in. I figured working on a documentation tool would be a good way to get started. |
tools/doc/html.js Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed and squashed.
515edb5 to 9b5344bCompareTrott commented Jun 23, 2018
From within
I'm going to add the If we can get all the code changes in that would result in |
rubys commented Jun 23, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Unlikely at this point as I have only added dependencies so far. Removing comes later.
Please explain?
Cool.
It is possible that I will be able to complete this tomorrow, but if not I don't expect to have much time Monday through Wednesday, but I should be able to wrap this up by next weekend. I'm happiest if changes are merged incrementally, but that may be asking a lot. I will say that this change should be easy to revert should it come to that. If not, should I have separate branches, or a single branch for the total set of changes? |
rubys commented Jun 23, 2018
Nevermind. Looks straightforward enough. |
Trott commented Jun 24, 2018
deleted: tools/doc/node_modules/define-properties/.editorconfig deleted: tools/doc/node_modules/define-properties/.eslintrc deleted: tools/doc/node_modules/define-properties/.jscs.json deleted: tools/doc/node_modules/define-properties/.npmignore deleted: tools/doc/node_modules/define-properties/.travis.yml deleted: tools/doc/node_modules/define-properties/CHANGELOG.md deleted: tools/doc/node_modules/define-properties/test/index.js deleted: tools/doc/node_modules/extend/.eslintrc deleted: tools/doc/node_modules/extend/.jscs.json deleted: tools/doc/node_modules/extend/.npmignore deleted: tools/doc/node_modules/extend/.travis.yml deleted: tools/doc/node_modules/extend/CHANGELOG.md deleted: tools/doc/node_modules/extend/component.json deleted: tools/doc/node_modules/foreach/.npmignore deleted: tools/doc/node_modules/foreach/component.json deleted: tools/doc/node_modules/foreach/test.js deleted: tools/doc/node_modules/is-nan/.eslintrc deleted: tools/doc/node_modules/is-nan/.jscs.json deleted: tools/doc/node_modules/is-nan/.npmignore deleted: tools/doc/node_modules/is-nan/.travis.yml deleted: tools/doc/node_modules/is-nan/CHANGELOG.md deleted: tools/doc/node_modules/is-nan/test/index.js deleted: tools/doc/node_modules/is-nan/test/shimmed.js deleted: tools/doc/node_modules/is-nan/test/tests.js deleted: tools/doc/node_modules/kebab-case/.editorconfig deleted: tools/doc/node_modules/kebab-case/.npmignore deleted: tools/doc/node_modules/kebab-case/.travis.yml deleted: tools/doc/node_modules/kebab-case/test.js deleted: tools/doc/node_modules/marked/.npmignore deleted: tools/doc/node_modules/marked/.travis.yml deleted: tools/doc/node_modules/marked/Gulpfile.js deleted: tools/doc/node_modules/marked/bower.json deleted: tools/doc/node_modules/marked/component.json deleted: tools/doc/node_modules/marked/doc/broken.md deleted: tools/doc/node_modules/marked/doc/todo.md deleted: tools/doc/node_modules/mdurl/CHANGELOG.md deleted: tools/doc/node_modules/object-keys/.editorconfig deleted: tools/doc/node_modules/object-keys/.eslintrc deleted: tools/doc/node_modules/object-keys/.jscs.json deleted: tools/doc/node_modules/object-keys/.travis.yml deleted: tools/doc/node_modules/object-keys/CHANGELOG.md deleted: tools/doc/node_modules/object-keys/test/index.js deleted: tools/doc/node_modules/trim/.npmignore deleted: tools/doc/node_modules/trim/History.md deleted: tools/doc/node_modules/trim/component.json deleted: tools/doc/node_modules/x-is-array/.npmignore deleted: tools/doc/node_modules/x-is-array/.travis.yml deleted: tools/doc/node_modules/x-is-array/test/index.js deleted: tools/doc/node_modules/x-is-string/.npmignore deleted: tools/doc/node_modules/x-is-string/.travis.yml deleted: tools/doc/node_modules/x-is-string/test/index.js deleted: tools/doc/node_modules/xtend/.jshintrc deleted: tools/doc/node_modules/xtend/.npmignore deleted: tools/doc/node_modules/xtend/test.js |
Trott commented Jun 24, 2018
Do it at whatever pace suits you. Having just one markdown processor in our dependencies is a nice-to-have for sure, but there's certainly no deadline or anything. Also, be aware that there's always the chance it won't get merged. There may be reasons for using
Maybe one PR/branch but multiple atomic/incremental commits? Just as long as every commit contains a fully working system. No commit should be broken, of course. |
tools/doc/html.js Outdated
vsemozhetbytJun 24, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually use a first uppercase letter and a period in the end in comments lately)
// Add class attributes to index navigation links.vsemozhetbyt commented Jun 24, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I've checked the diff between the results of this PR and the current master and it seems we have no significant changes so far, just these nits:
|
TimothyGu commented Jun 24, 2018
Is there a way to reuse the remark modules that are already included in our source tree? |
rubys commented Jun 24, 2018
Hit a speed bump. I've converted the code to the point where HTML is produced that resembles the input (always a good sign!) for all but one of the files. That file is A small test case: https://gist.github.com/rubys/a061064e0744a5792b3e8331b0dae37d The file in question is 52K lines. The problem is the size - processing either the first 26K or the last 26K lines works just fine. Possible solutions include increasing the heap size; processing this one file in chunks, building this file using other means, or aborting this effort. |
vsemozhetbyt commented Jun 24, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
BTW, |
rubys commented Jun 25, 2018
There appears to be very few LICENSE files in the node_modules themselves: |
Trott commented Jun 25, 2018
This is fantastic so far. Thanks for getting it moving so far so fast. I don't know what the right answer is to the 52K-line heap-exhaustion dilemma, but processing it in chunks seems like a promising idea given that the |
vsemozhetbyt commented Jun 25, 2018
It seems the cause of the issue is a collision between link references: now we join sources first and then parse/convert them, so all homonymous links are rewritten by the last one. If we could parse/convert the sources first and then join them, the issue may be fixed. |
rubys commented Jun 25, 2018
@vsemozhetbyt perhaps something like this: https://gist.github.com/rubys/4a140b5b61eab4cea34aca56098e900e ? Seems to be able to handle the full document without running out of heap space. Perhaps it could be cleaned up and landed separately as a fix for #20100 . |
tools/doc/package.json Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dependency intended for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed.
vsemozhetbyt commented Jun 28, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
What we also need to estimate is doc building time with various toolchains, as doc building is run on each CI and local tests. Currently on my machine, real 0m46.983s real 1m32.970sI.e. this approach is twice as slow. If we use it also for JSON generation, it can be three times as slow. However, it can be faster (or slower) with another |
rubys commented Jun 28, 2018
Some performance data: {gtoc: 31,parse: 207,firstHeader: 1,preprocessText: 8,preprocessElements: 131,buildToc: 46,remark2rehype: 26,raw: 262,serialize: 41 }See https://gist.github.com/rubys/4798e9c5418814c8810b295dfb5cbbaa for how I instrumented the code. Here's the command I used: I picked The biggest times are for parsing HTML ( I haven't looked closely at the JSON processing yet, but given that there is but a single occurrence of This one change won't eliminate the time difference between marked and remark tool chains, but would significantly reduce it. |
vsemozhetbyt left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only started to learn the toolchain and the new html.js, so this is just some nits so far) I will continue on Saturday if I find some more time)
tools/doc/html.js Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we do not use the file argument, maybe we can drop it here and in the recursive calls?
tools/doc/html.js Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just {filename } here and below?
tools/doc/html.js Outdated
vsemozhetbytJun 28, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is significant, but is it worth to make this change at the very end? We have some replacing below, and content.toString() is the biggest insertion here. Would the __GTOC__, analytics and docCreated insertions be faster if it is made before __CONTENT__ replacement? Because this will make .replace() calls operate on a smaller string.
vsemozhetbyt commented Jun 30, 2018
vsemozhetbyt commented Jun 30, 2018
rubys commented Jun 30, 2018
If #21568 is incorporated, I'll clean up and complete this pull request (for example, the increase in heap size can be removed). I'd rather wait on the JSON generation until a decision is made on remark vs marked, but if this one is approved, I will start on the JSON conversion. |
Trott commented Jul 1, 2018
Should the |
PR-URL: nodejs#21490 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Trott commented Jul 20, 2018
Trott commented Jul 20, 2018
|
rubys commented Jul 20, 2018
I'll try to clean it up as a part of #21697, which is now unblocked. |
This comment has been minimized.
This comment has been minimized.
Markdown interprets the syntax for optional arguments as a form of a link, so instead of trying to build up the contents using the node value, use grab the raw original markup instead. Fixes regression noted in nodejs#21490 (comment)
Markdown interprets the syntax for optional arguments as a form of a link, so instead of trying to build up the contents using the node value, use grab the raw original markup instead. Fixes regression noted in #21490 (comment) PR-URL: #21922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: #21490 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Markdown interprets the syntax for optional arguments as a form of a link, so instead of trying to build up the contents using the node value, use grab the raw original markup instead. Fixes regression noted in #21490 (comment) PR-URL: #21922 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Rich Trott <[email protected]>
vsemozhetbyt commented Jul 22, 2018 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@rubys Another breaking change: in headings, some optional arguments are linkified and next arguments disappear. Compare, for example, this doc heading before this PR landed and in the last Nightly (the last one is after the #21922 has been landed so that PR has not fixed this issue). |
ensure optional parameters are not treated as markedown links by replacing the children of headers nodes with a single text node containing the raw markup; Fixes issue identified in nodejs#21490 (comment)
ensure optional parameters are not treated as markedown links by replacing the children of headers nodes with a single text node containing the raw markup; Fixes issue identified in #21490 (comment) PR-URL: #21936 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
ensure optional parameters are not treated as markedown links by replacing the children of headers nodes with a single text node containing the raw markup; Fixes issue identified in #21490 (comment) PR-URL: #21936 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
* remove obsolete `node_modules/js-yaml/package.json` target * remove `@touch` since `npm ci` is always destructive PR-URL: nodejs#22399 Refs: nodejs#21802 Refs: nodejs#21490 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sam Ruby <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
* remove obsolete `node_modules/js-yaml/package.json` target * remove `@touch` since `npm ci` is always destructive PR-URL: #22399 Refs: #21802 Refs: #21490 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sam Ruby <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
* remove obsolete `node_modules/js-yaml/package.json` target * remove `@touch` since `npm ci` is always destructive PR-URL: #22399 Refs: #21802 Refs: #21490 Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Sam Ruby <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]>
First baby step towards implementing #21476
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes