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: build doc/api/all.html by combining generated HTML #21544

Closed
wants to merge 2 commits into from

Conversation

rubys
Copy link
Member

@rubys rubys commented Jun 26, 2018

Combine the toc and api contents from the generated doc/api/*.html
files. This ensures that the single page version of the documentation
exactly matches the individual pages.

Fixes #20100

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

Combine the toc and api contents from the generated doc/api/*.html
files.  This ensures that the single page version of the documentation
exactly matches the individual pages.

Fixes nodejs#20100
@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 Jun 26, 2018
@Trott
Copy link
Member

Trott commented Jun 26, 2018

/ping @nodejs/documentation especially @vsemozhetbyt

@Trott
Copy link
Member

Trott commented Jun 26, 2018

I added two commits, one to update package-lock.json (and add a missing dependency or two apparently) and another to update the LICENSE file. Those commits can be squashed out upon landing (or sooner).

@Trott Trott force-pushed the build_api_all_from_generated_html branch from b95d56a to 800713b Compare June 26, 2018 20:44
@vsemozhetbyt
Copy link
Contributor

I'll try to review this PR and #21490 this week, but I am not sure how thoroughly I will be able to do this: I have much less time recently and this is a whole new module set I am not experienced with. So if anybody knows this toolchain well, please, chime in)

@Trott
Copy link
Member

Trott commented Jun 26, 2018

The title of the all.html doc ends up being different. On master:

  <title>About this Documentation | Node.js v11.0.0 Documentation</title>

On this branch:

  <title>Node.js v11.0.0 Documentation</title>

For some reason, the links in the doc-version-picker are different (all.html vs. _toc.html).

also update Makefile to make apidocs depend on the tools
@rubys rubys force-pushed the build_api_all_from_generated_html branch from 800713b to 5e9cf65 Compare June 27, 2018 00:20
@rubys
Copy link
Member Author

rubys commented Jun 27, 2018

A brief overview of JSDOM: it is a faithful implementation of the browser DOM. So if anybody here has experience writing JavaScript in the browser, this should look very familiar. I said the overview would brief. :-)

As to doc-version-picker lnks: good catch! I've pushed a fix. Take a look at that fix to get an idea of what I mean by this code should look very familiar to anybody who has experience writing javascript in the browser.

As to the title: that change is intentional, though perhaps it could be improved. Previously, all.html was produced by concatenating the markdown input and running it through the publishing pipeline. For the table of contents and api content, this produced mostly the desirable results (with a few bugs as noted in this issue). In the case of the title, instead of concatenating the titles of each of the 50 documents, it picked the title of the first document.

Instead I went with the part of the title that is common to all documents. A good case could be made that this should be prefixed by something indicating 'single page'. Suggestions welcome. In any case, this would be a one line change - search for title.textContent =.

@jasnell
Copy link
Member

jasnell commented Jun 27, 2018

First off. Hi @rubys! Great to see you here! It's been quite a while, my friend!

Second, great to see work here. I won't be able to do a review until later this week.

@rubys
Copy link
Member Author

rubys commented Jun 27, 2018

Hi @jasnell! It hasn't really been that long. Looking forward to working with you again.

@TimothyGu
Copy link
Member

Hi, this PR is great. However, as one of the maintainers of jsdom I'm reluctant to recommend the inclusion of the entire package in Node.js tree, just because of its sheer size. I'd like to explore any possible alternatives that would allow us to accomplish the same goal in a more efficient way.

Moreover, as I noted in #21490 (comment), it seems like we would be duplicating a lot of npm modules we already include in the code base, like acorn. I know this is already somewhat an issue with npm and ESLint in tree, but it would be awesome if we could dedupe a bit (especially with ESLint, which is also in the tools directory).

@rubys
Copy link
Member Author

rubys commented Jun 27, 2018

@TimothyGu Perhaps you would prefer a version based on RegExps? Something like https://gist.github.com/rubys/26fb5b75d624098b021575c4b58874b0 perhaps? Much faster, zero dependencies, but perhaps a little less robust in that it depends on a number of HTML serialization choices; but overall that's probably acceptable for a tool that is only used at build time.

@TimothyGu
Copy link
Member

@rubys Yeah, I think that’s a better choice.

@jasnell
Copy link
Member

jasnell commented Jun 29, 2018

Zero dependencies is much better, even if a bit less robust.

@rubys
Copy link
Member Author

rubys commented Jun 29, 2018

Alternate version of this pull request, with zero dependencies can be found here: #21568

rubys added a commit to rubys/node that referenced this pull request Jun 30, 2018
Combine the toc and api contents from the generated doc/api/*.html
files. This ensures that the single page version of the documentation
exactly matches the individual pages.

Fixes nodejs#20100
@rubys
Copy link
Member Author

rubys commented Jul 3, 2018

Closing as #21568 was merged.

@rubys rubys closed this Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

doc: all.html is seriously broken link-wise
6 participants