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: Include links to source code in documentation #22405

Closed
wants to merge 9 commits into from
Closed
7 changes: 6 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -645,10 +645,15 @@ out/doc/api/assets/%: doc/api_assets/% out/doc/api/assets
run-npm-ci = $(PWD)/$(NPM) ci

gen-api = tools/doc/generate.js --node-version=$(FULLVERSION) \
--apilinks=out/apilinks.json \
--analytics=$(DOCS_ANALYTICS) $< --output-directory=out/doc/api
gen-apilink = tools/doc/apilinks.js $(wildcard lib/*.js) > $@

out/apilinks.json: $(wildcard lib/*.js) tools/doc/apilinks.js
$(call available-node, $(gen-apilink))

out/doc/api/%.json out/doc/api/%.html: doc/api/%.md tools/doc/generate.js \
tools/doc/html.js tools/doc/json.js
tools/doc/html.js tools/doc/json.js | out/apilinks.json
$(call available-node, $(gen-api))

out/doc/api/all.html: $(apidocs_html) tools/doc/allhtml.js
Expand Down
5 changes: 5 additions & 0 deletions doc/api_assets/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,11 @@ h2, h3, h4, h5 {
padding-right: 40px;
}

.srclink {
float: right;
font-size: smaller;
}

h1 span, h2 span, h3 span, h4 span {
position: absolute;
display: block;
Expand Down
39 changes: 39 additions & 0 deletions test/doctool/test-apilinks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
'use strict';

require('../common');
const fixtures = require('../common/fixtures');
const fs = require('fs');
const assert = require('assert');
const path = require('path');
const { execFileSync } = require('child_process');

const script = path.join(__dirname, '..', '..', 'tools', 'doc', 'apilinks.js');

const apilinks = fixtures.path('apilinks');
fs.readdirSync(apilinks).forEach((fixture) => {
if (!fixture.endsWith('.js')) return;
const file = path.join(apilinks, fixture);

const expectedContent = fs.readFileSync(file + 'on', 'utf8');

const output = execFileSync(
process.execPath,
[script, file],
{ encoding: 'utf-8' }
);

const expectedLinks = JSON.parse(expectedContent);
const actualLinks = JSON.parse(output);

for (const [k, v] of Object.entries(expectedLinks)) {
assert.ok(k in actualLinks, `link not found: ${k}`);
assert.ok(actualLinks[k].endsWith('/' + v),
`link ${actualLinks[k]} expected to end with ${v}`);
delete actualLinks[k];
}

assert.strictEqual(
Object.keys(actualLinks).length, 0,
`unexpected links returned ${JSON.stringify(actualLinks)}`
);
});
2 changes: 1 addition & 1 deletion test/doctool/test-doctool-html.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function toHTML({ input, filename, nodeVersion, analytics }, cb) {
.use(html.firstHeader)
.use(html.preprocessText)
.use(html.preprocessElements, { filename })
.use(html.buildToc, { filename })
.use(html.buildToc, { filename, apilinks: {} })
.use(remark2rehype, { allowDangerousHTML: true })
.use(raw)
.use(htmlStringify)
Expand Down
12 changes: 12 additions & 0 deletions test/fixtures/apilinks/buffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
'use strict';

// Buffer instance methods are exported as 'buf'.

function Buffer() {
}

Buffer.prototype.instanceMethod = function() {}

module.exports = {
Buffer
};
4 changes: 4 additions & 0 deletions test/fixtures/apilinks/buffer.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"buffer.Buffer": "buffer.js#L5",
"buf.instanceMethod": "buffer.js#L8"
}
11 changes: 11 additions & 0 deletions test/fixtures/apilinks/mod.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
'use strict';

// A module may export one or more methods.

function foo() {
}


module.exports = {
foo
};
3 changes: 3 additions & 0 deletions test/fixtures/apilinks/mod.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"mod.foo": "mod.js#L5"
}
13 changes: 13 additions & 0 deletions test/fixtures/apilinks/prototype.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';

// An exported class using classic prototype syntax.

function Class() {
}

Class.classMethod = function() {}
Class.prototype.instanceMethod = function() {}

module.exports = {
Class
};
5 changes: 5 additions & 0 deletions test/fixtures/apilinks/prototype.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"prototype.Class": "prototype.js#L5",
"Class.classMethod": "prototype.js#L8",
"class.instanceMethod": "prototype.js#L9"
}
141 changes: 141 additions & 0 deletions tools/doc/apilinks.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
'use strict';

// Scan API sources for definitions.
//
// Note the output is produced based on a world class parser, adherence to
// conventions, and a bit of guess work. Examples:
//
// * We scan for top level module.exports statements, and determine what
// is exported by looking at the source code only (i.e., we don't do
// an eval). If exports include `Foo`, it probably is a class, whereas
// if what is exported is `constants` it probably is prefixed by the
// basename of the source file (e.g., `zlib`), unless that source file is
// `buffer.js`, in which case the name is just `buf`. unless the constant
// is `kMaxLength`, in which case it is `buffer`.
//
// * We scan for top level definitions for those exports, handling
// most common cases (e.g., `X.prototype.foo =`, `X.foo =`,
// `function X(...) {...}`). Over time, we expect to handle more
// cases (example: ES2015 class definitions).

const acorn = require('../../deps/acorn');
const fs = require('fs');
const path = require('path');
const child_process = require('child_process');

// Run a command, capturing stdout, ignoring errors.
function execSync(command) {
try {
return child_process.execSync(
command,
{ stdio: ['ignore', null, 'ignore'] }
).toString().trim();
} catch {
return '';
}
}

// Determine orign repo and tag (or hash) of the most recent commit.
const local_branch = execSync('git name-rev --name-only HEAD');
const tracking_remote = execSync(`git config branch.${local_branch}.remote`);
const remote_url = execSync(`git config remote.${tracking_remote}.url`);
const repo = (remote_url.match(/(\w+\/\w+)\.git\r?\n?$/) ||
['', 'nodejs/node'])[1];

const hash = execSync('git log -1 --pretty=%H') || 'master';
const tag = execSync(`git describe --contains ${hash}`).split('\n')[0] || hash;

// Extract definitions from each file specified.
const definition = {};
process.argv.slice(2).forEach((file) => {
const basename = path.basename(file, '.js');

// Parse source.
const source = fs.readFileSync(file, 'utf8');
const ast = acorn.parse(source, { ecmaVersion: 10, locations: true });
const program = ast.body;

// Scan for exports.
const exported = { constructors: [], identifiers: [] };
program.forEach((statement) => {
if (statement.type !== 'ExpressionStatement') return;
const expr = statement.expression;
if (expr.type !== 'AssignmentExpression') return;

let lhs = expr.left;
if (expr.left.object.type === 'MemberExpression') lhs = lhs.object;
if (lhs.type !== 'MemberExpression') return;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about exports shortcut? Do we exclude it?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we use it, it needs to be added. This PR was meant to be enough to validate the approach. If we are happy with the approach, we will need to go back and see what additional code patterns we will have to look for.

if (lhs.object.name !== 'module') return;
if (lhs.property.name !== 'exports') return;

let rhs = expr.right;
while (rhs.type === 'AssignmentExpression') rhs = rhs.right;

if (rhs.type === 'NewExpression') {
exported.constructors.push(rhs.callee.name);
} else if (rhs.type === 'ObjectExpression') {
rhs.properties.forEach((property) => {
if (property.value.type === 'Identifier') {
exported.identifiers.push(property.value.name);
if (/^[A-Z]/.test(property.value.name[0])) {
exported.constructors.push(property.value.name);
}
}
});
} else if (rhs.type === 'Identifier') {
exported.identifiers.push(rhs.name);
}
});

// Scan for definitions matching those exports; currently supports:
//
// ClassName.foo = ...;
// ClassName.prototype.foo = ...;
// function Identifier(...) {...};
//
const link = `https://github.com/${repo}/blob/${tag}/` +
path.relative('.', file).replace(/\\/g, '/');

program.forEach((statement) => {
if (statement.type === 'ExpressionStatement') {
const expr = statement.expression;
if (expr.type !== 'AssignmentExpression') return;
if (expr.left.type !== 'MemberExpression') return;

let object;
if (expr.left.object.type === 'MemberExpression') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another ignorable performance nit: maybe we should cache such things more; for example, we retrieve expr.left.object many times here. But I am not sure if the gain is worth to create medium variables.

if (expr.left.object.property.name !== 'prototype') return;
object = expr.left.object.object;
} else if (expr.left.object.type === 'Identifier') {
object = expr.left.object;
} else {
return;
}

if (!exported.constructors.includes(object.name)) return;

let objectName = object.name;
if (expr.left.object.type === 'MemberExpression') {
objectName = objectName.toLowerCase();
if (objectName === 'buffer') objectName = 'buf';
}

let name = expr.left.property.name;
if (expr.left.computed) {
name = `${objectName}[${name}]`;
} else {
name = `${objectName}.${name}`;
}

definition[name] = `${link}#L${statement.loc.start.line}`;
} else if (statement.type === 'FunctionDeclaration') {
const name = statement.id.name;
if (!exported.identifiers.includes(name)) return;
if (basename.startsWith('_')) return;
definition[`${basename}.${name}`] =
`${link}#L${statement.loc.start.line}`;
}
});
});

console.log(JSON.stringify(definition, null, 2));
7 changes: 6 additions & 1 deletion tools/doc/generate.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ let filename = null;
let nodeVersion = null;
let analytics = null;
let outputDir = null;
let apilinks = {};

args.forEach(function(arg) {
if (!arg.startsWith('--')) {
Expand All @@ -50,6 +51,10 @@ args.forEach(function(arg) {
analytics = arg.replace(/^--analytics=/, '');
} else if (arg.startsWith('--output-directory=')) {
outputDir = arg.replace(/^--output-directory=/, '');
} else if (arg.startsWith('--apilinks=')) {
apilinks = JSON.parse(
fs.readFileSync(arg.replace(/^--apilinks=/, ''), 'utf8')
);
}
});

Expand All @@ -71,7 +76,7 @@ fs.readFile(filename, 'utf8', (er, input) => {
.use(json.jsonAPI, { filename })
.use(html.firstHeader)
.use(html.preprocessElements, { filename })
.use(html.buildToc, { filename })
.use(html.buildToc, { filename, apilinks })
.use(remark2rehype, { allowDangerousHTML: true })
.use(raw)
.use(htmlStringify)
Expand Down
7 changes: 6 additions & 1 deletion tools/doc/html.js
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ function versionSort(a, b) {
return +b.match(numberRe)[0] - +a.match(numberRe)[0];
}

function buildToc({ filename }) {
function buildToc({ filename, apilinks }) {
return (tree, file) => {
const startIncludeRefRE = /^\s*<!-- \[start-include:(.+)\] -->\s*$/;
const endIncludeRefRE = /^\s*<!-- \[end-include:.+\] -->\s*$/;
Expand Down Expand Up @@ -376,6 +376,11 @@ function buildToc({ filename }) {
`id="${headingText}">#</a></span>`;
}

const api = headingText.replace(/^.*:\s+/, '').replace(/\(.*/, '');
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Aug 24, 2018

Choose a reason for hiding this comment

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

  1. Nit: maybe it is worth to cache apilinks[api] to not retrieve twice:
      const apilink =
        apilinks[headingText.replace(/^.*:\s+/, '').replace(/\(.*/, '')];
      if (apilink) {
        anchor = `<a class="srclink" href=${apilink}>[src]</a>${anchor}`;
      }
  1. If apilinks is an empty object (no --apilinks flag is provided), we do some unneeded job here. Maybe transfer null instead of {} and check the null before all the headingText.replace business? Feel free to ignore if this is overcomplication with too little performance gain.

if (apilinks[api]) {
anchor = `<a class="srclink" href=${apilinks[api]}>[src]</a>${anchor}`;
}

node.children.push({ type: 'html', value: anchor });
});

Expand Down