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: simplify HTML generation #20307

Closed
wants to merge 1 commit into from
Closed

tools: simplify HTML generation #20307

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 25, 2018

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

In tools/doc/html.js we have 3 operations that have little benefit but complicate the file significantly.

  1. Asynchronous loading of doc/template.html.
  2. Asynchronous loading of doc/api/_toc.md.
  3. Requiring tools/doc/preprocess.js and calling its main function.

While doing 1 and 2 asynchronously can have a performance benefit, it is rather small (both files are ~1.5 KB). Besides, the downsides are significant:

  1. tools/doc/html.js is required in tools/doc/generate.js and test/doctool/test-doctool-html.js. While the first script calls the main function once per requiring, the second script calls it many times and all these times the same doc/template.html is loaded needlessly.

  2. Asynchronous loading of doc/ap/_toc.md makes the script overcomplicated to the extent of this comment:

// TODO(chrisdickinson): never stop vomiting / fix this.

  1. The main aim of the tools/doc/preprocess.js is processing @include instructions. Also, as one goes, it strips special comments. There are no @include instructions in doc/ap/_toc.md so the requiring this module is excessiveness.

This PR try to eliminate all downsides in this way:

  1. As we have only one HTML template, we can exclude it as an option from cli chain, incorporate its path in tools/doc/html.js and preload it synchronously once per module initialization.

  2. We can preload doc/api/_toc.md synchronously and delete a huge async machinery.

  3. We can replace tools/doc/preprocess.js requiring with one-liner comment stripping in place.

This PR reduces the code almost by 50 lines producing the same result.

Please, ignore any remaining stylistic issues while reviewing the changes: I am going to modernize and optimize the whole script after this PR. This change is singled out to not mingle logic refactoring with many small changes scattering over the file.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Apr 25, 2018
@vsemozhetbyt

This comment has been minimized.

@richardlau
Copy link
Member

Please, ignore any remaining stylistic issues while reviewing the changes

Aren't these files linted?

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 25, 2018

@richardlau I've meant the old rudiments that are accepted by the linter but are usually fixed now: var and so on. They will be addressed later.

@vsemozhetbyt
Copy link
Contributor Author

@addaleax Sorry, to be sure: your approval has been made in 2 minutes after the PR had been filed: is it intended?

@addaleax
Copy link
Member

@vsemozhetbyt Yes, that was an actual review. :) If you want something more explicit: This looks correct to me, and if CI passes then I’m happy.

And it’s awesome that you’re cleaning this mess up, so that’s very much appreciated too. :)

@vsemozhetbyt
Copy link
Contributor Author

I am not sure if we can consider a fast-tracking here: this is a code change and it touches build script. But it is contained in doc domain only and does not change anything in the output and results. So if anybody does approve fast-tracking, please, add 👍 here and I will start to clean the script on after this landed.

@vsemozhetbyt
Copy link
Contributor Author

I've reverted an inadvertent comment change.
CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite/590/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2018
@vsemozhetbyt
Copy link
Contributor Author

Landed in ad6a65b

@vsemozhetbyt vsemozhetbyt deleted the tools-simplify-html-generation branch April 27, 2018 21:53
vsemozhetbyt added a commit that referenced this pull request Apr 27, 2018
PR-URL: #20307
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
PR-URL: #20307
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
MylesBorins pushed a commit that referenced this pull request Aug 17, 2018
PR-URL: #20307
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. 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.

6 participants