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 #21568

Closed

Conversation

rubys
Copy link
Member

@rubys rubys commented Jun 27, 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

This pull request differs from #21544 in that it is implemented using regular expressions and has no dependencies. See #21544 (comment) for more context.

If this pull request is merged, the other will be deleted.

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

@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 27, 2018
@rubys rubys changed the title tools: build doc/api/all.html by combining generated HTML #21544 tools: build doc/api/all.html by combining generated HTML Jun 27, 2018
@Trott
Copy link
Member

Trott commented Jun 28, 2018

@TimothyGu @vsemozhetbyt

@Trott
Copy link
Member

Trott commented Jun 29, 2018

@nodejs/documentation It would be a good idea for someone to build all.html with this and compare it to what's in master. The diff should show some broken links being fixed but otherwise there should be no significant changes.

@rubys rubys force-pushed the build_api_all_from_generated_html2 branch from 338d466 to 0b9ff5f Compare June 30, 2018 00:36
@rubys
Copy link
Member Author

rubys commented Jun 30, 2018

I've made a few fixes to reduce the differences, rebased to master, and here is a current set of differences: https://gist.github.com/rubys/bae70ed087efec625ac04823aaaf803e

Summary:

  • title is different; instead of picking the first title the new title is the part that is common to all titles. This could be changed, but I would still not recommend that it be the first title.
  • Some whitespace changes - nothing that would affect rendering
  • No include comments; something like that could be added if required, but likely would differ in that it would make more sense to reference the generated html files instead of the md source.
  • An odd change in api stability section; for reasons I don't understand the generation of this section added links previously. Links that are not present on the documentation page. Compare the current node documentation as published publicly: all.html vs documentation.html.
  • The previously generated all.html would sometimes link to definitions within the all.html page, and other times link to definitions on the separate page, and sometimes link to the wrong thing entirely. This has been changed to always link within the same page, and always to exactly what was linked to in the separate pages.

@vsemozhetbyt
Copy link
Contributor

The same results here)

An odd change in api stability section...

This is OK. Due to this code:

node/tools/doc/html.js

Lines 209 to 216 in 1bf42f4

// Do not link to the section we are already in.
const noLinking = filename === 'documentation' &&
heading !== null && heading.text === 'Stability Index';
token.text = `<div class="api_stability api_stability_${number}">` +
(noLinking ? '' :
'<a href="documentation.html#documentation_stability_index">') +
`${prefix} ${number}${noLinking ? '' : '</a>'}${explication}</div>`
.replace(/\n/g, ' ');

these links were skipped in documentation.html but wrongly reappeared in all.html. The new result is more consistent, I think.

let contents = '';
let apicontent = '';

// Identify files that should be skipped. As files are processed, they
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 before "As".

.replace('api-section-_toc', 'api-section-all')
.replace('data-id="_toc"', 'data-id="all"');

// clean up the title.
Copy link
Contributor

Choose a reason for hiding this comment

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

clean -> Clean ?

'<ul>\n' + contents + '</ul>\n' +
all.slice(tocStart.index + tocStart[0].length);

// Replace apicontent with the contenated set of apicontents from each source.
Copy link
Contributor

Choose a reason for hiding this comment

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

contenated -> concatenated?

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 rubys force-pushed the build_api_all_from_generated_html2 branch from 0b9ff5f to 6dac7db Compare June 30, 2018 11:14
@rubys
Copy link
Member Author

rubys commented Jun 30, 2018

Can somebody spot the CI error for me? I don't see it. I searched for 'error' and 'fail'.

@vsemozhetbyt
Copy link
Contributor

You can also search for "not ok" in raw log: https://api.travis-ci.com/v3/job/132202841/log.txt
The test failure seems unrelated:
not ok 2146 es-module/test-esm-loader-invalid-format

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

Refs: #21605

@vsemozhetbyt
Copy link
Contributor

#21605 is landed.
New full CI: https://ci.nodejs.org/job/node-test-pull-request/15684/

@Trott
Copy link
Member

Trott commented Jun 30, 2018

@vsemozhetbyt
Copy link
Contributor

I shall land in some hours if there are no objections.

@vsemozhetbyt
Copy link
Contributor

Landed in f85962f
Thank you!

vsemozhetbyt pushed a commit that referenced this pull request Jul 2, 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.

PR-URL: #21568
Fixes: #20100
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Maybe we can do the same for the all.json so that we could completely rid of all.md. This can be done by JSON.parse() + combine new object + JSON.stringify() or just manually concatenate the new stringified serialization, not sure what would be easier or more effective. This may be semver-major as it may change the JSON structure users rely on programmatically.

@rubys
Copy link
Member Author

rubys commented Jul 2, 2018

@vsemozhetbyt in the process, we could also get rid of preprocess.js and therefore processIncludes. Please open an issue and I'll work on a pull request.

@vsemozhetbyt
Copy link
Contributor

We usually do not require an issue for a PR. Feel free to pace as you feel comfortable)

@Trott
Copy link
Member

Trott commented Jul 2, 2018

Awesome to see "Contributor" on @rubys's comments etc. everywhere now. 🎉

targos pushed a commit that referenced this pull request Jul 3, 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.

PR-URL: #21568
Fixes: #20100
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
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
5 participants