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

doc, tools: refactor html generation #6974

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ out/doc/api/%.json: doc/api/%.md
[ -x $(NODE) ] && $(NODE) $(gen-json) || node $(gen-json)

# check if ./node is actually set, else use user pre-installed binary
gen-html = tools/doc/generate.js --node-version=$(FULLVERSION) --format=html --template=doc/template.html $< > $@
gen-html = tools/doc/generate.js --node-version=$(FULLVERSION) --format=html --template=doc/template.html $<
out/doc/api/%.html: doc/api/%.md
[ -x $(NODE) ] && $(NODE) $(gen-html) || node $(gen-html)

Expand Down
11 changes: 11 additions & 0 deletions doc/guides/_toc.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
* [About these Docs](documentation.html)
* [Usage & Example](synopsis.html)

<div class="line"></div>

* [Building Node With Ninja](building-node-with-ninja.html)

<div class="line"></div>

* [GitHub Repo & Issue Tracker](https://github.com/nodejs/node)
* [Mailing List](http://groups.google.com/group/nodejs)
44 changes: 34 additions & 10 deletions test/doctool/test-doctool-html.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ const assert = require('assert');
const fs = require('fs');
const path = require('path');

const html = require('../../tools/doc/html.js');
common.globalCheck = false;
Copy link
Member

Choose a reason for hiding this comment

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

This should best come with a comment stating the names of the additional globals


const processIncludes = require('../../tools/doc/preprocess.js');
const html = require('../../tools/doc/html.js').toHTML;

// Test data is a list of objects with two properties.
// The file property is the file path.
Expand Down Expand Up @@ -53,22 +56,43 @@ const testData = [
'<p>Describe <code>Something</code> in more detail here. ' +
'</p>'
},
{
file: path.join(common.fixturesDir, 'doc_with_includes.md'),
html: '<!-- [start-include:doc_inc_1.md] -->' +
'<p>Look <a href="doc_inc_2.html#doc_inc_2_foobar">here</a>!</p>' +
'<!-- [end-include:doc_inc_1.md] -->' +
'<!-- [start-include:doc_inc_2.md] -->' +
'<h1>foobar<span><a class="mark" href="#doc_inc_2_foobar" ' +
'id="doc_inc_2_foobar">#</a></span></h1>' +
'<p>I exist and am being linked to.</p>' +
'<!-- [end-include:doc_inc_2.md] -->'
},
];

testData.forEach(function(item) {
// Normalize expected data by stripping whitespace
const expected = item.html.replace(/\s/g, '');

fs.readFile(item.file, 'utf8', common.mustCall(function(err, input) {
fs.readFile(item.file, 'utf8', common.mustCall((err, input) => {
assert.ifError(err);
html(input, 'foo', 'doc/template.html',
common.mustCall(function(err, output) {
assert.ifError(err);
processIncludes(item.file, input, common.mustCall((err, preprocessed) => {
assert.ifError(err);

html(
{
input: preprocessed,
filename: 'foo',
template: 'doc/template.html',
nodeVersion: process.version,
},
common.mustCall((err, output) => {
assert.ifError(err);

const actual = output.replace(/\s/g, '');
// Assert that the input stripped of all whitespace contains the
// expected list
assert.notEqual(actual.indexOf(expected), -1);
}));
const actual = output.replace(/\s/g, '');
// Assert that the input stripped of all whitespace contains the
// expected list
assert.notEqual(actual.indexOf(expected), -1);
}));
}));
}));
});
14 changes: 10 additions & 4 deletions tools/doc/generate.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

const processIncludes = require('./preprocess.js');
const fs = require('fs');
const path = require('path');
const toHTML = require('./html.js').toHTML;

// parse the args.
// Don't use nopt or whatever for this. It's simple enough.
Expand Down Expand Up @@ -48,11 +50,15 @@ function next(er, input) {
break;

case 'html':
require('./html.js')(input, inputFile, template, nodeVersion,
function(er, html) {
if (er) throw er;
console.log(html);
toHTML(input, inputFile, template, nodeVersion, (er, html) => {
if (er) throw er;
const filename = `./out/doc/api/${path.basename(inputFile, 'md')}html`;
Copy link
Member

Choose a reason for hiding this comment

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

I am not a 100 % sure I like having the output paths hardcoded here:

  • Generally, the doctool can be run on any .md file and is somewhat agnostic about the project it belogs to (i.e. it doesn’t even have to be Node.js docs that are being processed), and I’d prefer to keep it more or less that way
  • This would make it harder to include docs from other directories if that’s desired at some point (doc/topics being a likely candidate?)
  • Having the Makefile output include the output file makes it easier to reason about what is actually happening during building

So… what are the advantages of doing it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. it can with that. basically you define the behaviour of the guide, html etc. rendering within that switch case. It would of course be possible to have a flag for the output in the makefile. A flag okay, having unix control flow through piping not so okay - especially since we have all the necessary tools o.board. So +1 for modularity through, flags, configs etc. We would need to decide what it should be though. Currently and previously the configuration basically happened in the switch case.
  2. as noted above, it makes it even easier. We can improve though.
  3. The makefile around there is more verbose than having it in JS, since it only consist of variables that will be iterated over files, which is probably baggage from the early days. There is no real reason not to do in JS to gain more flexibility and maintainability.
  4. There is no apparent advantage but also there is no real change in there. The advantage will be that I can branch for 'guides' and use components 'html' doesn't need. Namely We will need a custom component for the _toc since it will be nested.

fs.writeFile(filename, html, (err) => {
if (err)
console.log(err);
console.log('generated:', filename);
});
});
break;

default:
Expand Down
Loading