From 70c14e6b998cbb44ff96019b4c181c8afc3e9da9 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 28 Dec 2020 17:38:00 -0800 Subject: [PATCH] doc: generator code health (#4840) --- utils/doclint/Documentation.js | 19 ++- utils/doclint/MDBuilder.js | 160 +++++++++++++++++-------- utils/doclint/cli.js | 27 +++-- utils/doclint/generateApiJson.js | 11 +- utils/doclint/missingDocs.js | 23 ++-- utils/doclint/test/missingDocs.spec.js | 10 +- utils/doclint/test/test-api.md | 12 +- utils/generate_types/index.js | 10 +- utils/{parse_md.js => markdown.js} | 154 +++++++++--------------- 9 files changed, 221 insertions(+), 205 deletions(-) rename utils/{parse_md.js => markdown.js} (60%) diff --git a/utils/doclint/Documentation.js b/utils/doclint/Documentation.js index dc378ee27c..015e143bff 100644 --- a/utils/doclint/Documentation.js +++ b/utils/doclint/Documentation.js @@ -16,14 +16,7 @@ // @ts-check -/** @typedef {{ - * type: 'text' | 'li' | 'code' | 'gen' | 'h1' | 'h2' | 'h3' | 'h4', - * text?: string, - * codeLang?: string, - * lines?: string[], - * liType?: 'default' | 'bullet' | 'ordinal', - * children?: MarkdownNode[] - * }} MarkdownNode */ +/** @typedef {import('../markdown').MarkdownNode} MarkdownNode */ class Documentation { /** @@ -64,10 +57,6 @@ Documentation.Class = class { /** @type {!Array} */ this.propertiesArray = []; /** @type {!Map} */ - this.options = new Map(); - /** @type {!Array} */ - this.optionsArray = []; - /** @type {!Map} */ this.methods = new Map(); /** @type {!Array} */ this.methodsArray = []; @@ -135,6 +124,9 @@ Documentation.Class = class { } } + /** + * @param {function(Documentation.Member|Documentation.Class): void} visitor + */ visit(visitor) { visitor(this); for (const p of this.propertiesArray) @@ -204,6 +196,9 @@ Documentation.Member = class { return new Documentation.Member('event', name, type, [], spec); } + /** + * @param {function(Documentation.Member|Documentation.Class): void} visitor + */ visit(visitor) { visitor(this); if (this.type) diff --git a/utils/doclint/MDBuilder.js b/utils/doclint/MDBuilder.js index a66a980ce5..b8f04b0509 100644 --- a/utils/doclint/MDBuilder.js +++ b/utils/doclint/MDBuilder.js @@ -16,17 +16,22 @@ // @ts-check -const { parseArgument, renderMd, clone } = require('../parse_md'); +const fs = require('fs'); +const md = require('../markdown'); const Documentation = require('./Documentation'); -/** @typedef {import('./Documentation').MarkdownNode} MarkdownNode */ +/** @typedef {import('../markdown').MarkdownNode} MarkdownNode */ class MDOutline { /** - * @param {MarkdownNode[]} api + * @param {string} bodyPath + * @param {string=} paramsPath * @param {string=} links */ - constructor(api, links = '') { + constructor(bodyPath, paramsPath, links = '') { + const body = md.parse(fs.readFileSync(bodyPath).toString()); + const params = paramsPath ? md.parse(fs.readFileSync(paramsPath).toString()) : null; + const api = params ? applyTemplates(body, params) : body; this.classesArray = /** @type {Documentation.Class[]} */ []; this.classes = /** @type {Map} */ new Map(); for (const clazz of api) { @@ -42,8 +47,32 @@ class MDOutline { linksMap.set(new RegExp('\\[' + match[1] + '\\]', 'g'), { href: match[2], label: match[3] }); } this.signatures = this._generateComments(linksMap); + this.documentation = new Documentation(this.classesArray); } + /** + * @param {string[]} errors + */ + copyDocsFromSuperclasses(errors) { + for (const [name, clazz] of this.documentation.classes.entries()) { + clazz.validateOrder(errors, clazz); + + if (!clazz.extends || clazz.extends === 'EventEmitter' || clazz.extends === 'Error') + continue; + const superClass = this.documentation.classes.get(clazz.extends); + if (!superClass) { + errors.push(`Undefined superclass: ${superClass} in ${name}`); + continue; + } + for (const memberName of clazz.members.keys()) { + if (superClass.members.has(memberName)) + errors.push(`Member documentation overrides base: ${name}.${memberName} over ${clazz.extends}.${memberName}`); + } + + clazz.membersArray = [...clazz.membersArray, ...superClass.membersArray]; + clazz.index(); + } + } /** * @param {Map} linksMap */ @@ -81,7 +110,7 @@ class MDOutline { } for (const clazz of this.classesArray) - clazz.visit(item => patchSignatures(item.spec, signatures)); + clazz.visit(item => patchLinks(item.spec, signatures)); for (const clazz of this.classesArray) clazz.visit(item => item.comment = renderCommentsForSourceCode(item.spec, linksMap)); return signatures; @@ -120,8 +149,8 @@ function extractComments(item) { * @param {Map} linksMap */ function renderCommentsForSourceCode(spec, linksMap) { - const comments = (spec || []).filter(n => n.type !== 'gen' && !n.type.startsWith('h') && (n.type !== 'li' || n.liType !== 'default')).map(c => clone(c)); - const visit = node => { + const comments = (spec || []).filter(n => n.type !== 'gen' && !n.type.startsWith('h') && (n.type !== 'li' || n.liType !== 'default')).map(c => md.clone(c)); + md.visitAll(comments, node => { if (node.text) { for (const [regex, { href, label }] of linksMap) node.text = node.text.replace(regex, `[${label}](${href})`); @@ -133,27 +162,21 @@ function renderCommentsForSourceCode(spec, linksMap) { } if (node.liType === 'bullet') node.liType = 'default'; - for (const child of node.children || []) - visit(child); - }; - for (const node of comments) - visit(node); - return renderMd(comments, 10000); - - // [`frame.waitForFunction(pageFunction[, arg, options])`](#framewaitforfunctionpagefunction-arg-options) + }); + return md.render(comments); } /** * @param {MarkdownNode[]} spec * @param {Map} [signatures] */ -function patchSignatures(spec, signatures) { +function patchLinks(spec, signatures) { for (const node of spec || []) { if (node.type === 'text') - node.text = patchSignaturesInText(node.text, signatures); + node.text = patchLinksInText(node.text, signatures); if (node.type === 'li') { - node.text = patchSignaturesInText(node.text, signatures); - patchSignatures(node.children, signatures); + node.text = patchLinksInText(node.text, signatures); + patchLinks(node.children, signatures); } } } @@ -171,7 +194,7 @@ function createLink(text) { * @param {string} comment * @param {Map} signatures */ -function patchSignaturesInText(comment, signatures) { +function patchLinksInText(comment, signatures) { if (!signatures) return comment; comment = comment.replace(/\[`(event|method|property):\s(JS|CDP|[A-Z])([^.]+)\.([^`]+)`\]\(\)/g, (match, type, clazzPrefix, clazz, name) => { @@ -291,36 +314,79 @@ function guessRequired(comment) { return required; } -module.exports = /** - * @param {any} api - * @param {boolean=} copyDocsFromSuperClasses + * @param {MarkdownNode[]} body + * @param {MarkdownNode[]} params */ - function(api, copyDocsFromSuperClasses = false, links = '') { - const errors = []; - const outline = new MDOutline(api, links); - const documentation = new Documentation(outline.classesArray); +function applyTemplates(body, params) { + const paramsMap = new Map(); + for (const node of params) + paramsMap.set('%%-' + node.text + '-%%', node); - if (copyDocsFromSuperClasses) { - // Push base class documentation to derived classes. - for (const [name, clazz] of documentation.classes.entries()) { - clazz.validateOrder(errors, clazz); - - if (!clazz.extends || clazz.extends === 'EventEmitter' || clazz.extends === 'Error') - continue; - const superClass = documentation.classes.get(clazz.extends); - if (!superClass) { - errors.push(`Undefined superclass: ${superClass} in ${name}`); - continue; + const visit = (node, parent) => { + if (node.text && node.text.includes('-inline- = %%')) { + const [name, key] = node.text.split('-inline- = '); + const list = paramsMap.get(key); + if (!list) + throw new Error('Bad template: ' + key); + for (const prop of list.children) { + const template = paramsMap.get(prop.text); + if (!template) + throw new Error('Bad template: ' + prop.text); + const { name: argName } = parseArgument(template.children[0].text); + parent.children.push({ + type: node.type, + text: name + argName, + children: template.children.map(c => md.clone(c)) + }); } - for (const memberName of clazz.members.keys()) { - if (superClass.members.has(memberName)) - errors.push(`Member documentation overrides base: ${name}.${memberName} over ${clazz.extends}.${memberName}`); - } - - clazz.membersArray = [...clazz.membersArray, ...superClass.membersArray]; - clazz.index(); + } else if (node.text && node.text.includes(' = %%')) { + const [name, key] = node.text.split(' = '); + node.text = name; + const template = paramsMap.get(key); + if (!template) + throw new Error('Bad template: ' + key); + node.children.push(...template.children.map(c => md.clone(c))); } + for (const child of node.children || []) + visit(child, node); + if (node.children) + node.children = node.children.filter(child => !child.text || !child.text.includes('-inline- = %%')); + }; + + for (const node of body) + visit(node, null); + + return body; +} + +/** + * @param {string} line + * @returns {{ name: string, type: string, text: string }} + */ +function parseArgument(line) { + let match = line.match(/^`([^`]+)` (.*)/); + if (!match) + match = line.match(/^(returns): (.*)/); + if (!match) + match = line.match(/^(type): (.*)/); + if (!match) + throw new Error('Invalid argument: ' + line); + const name = match[1]; + const remainder = match[2]; + if (!remainder.startsWith('<')) + throw new Error('Bad argument: ' + remainder); + let depth = 0; + for (let i = 0; i < remainder.length; ++i) { + const c = remainder.charAt(i); + if (c === '<') + ++depth; + if (c === '>') + --depth; + if (depth === 0) + return { name, type: remainder.substring(1, i), text: remainder.substring(i + 2) }; } - return { documentation, errors, outline }; -}; + throw new Error('Should not be reached'); +} + +module.exports = { MDOutline }; diff --git a/utils/doclint/cli.js b/utils/doclint/cli.js index 00cf5fb7f9..b49fee09c7 100755 --- a/utils/doclint/cli.js +++ b/utils/doclint/cli.js @@ -21,13 +21,14 @@ const playwright = require('../../'); const fs = require('fs'); const path = require('path'); const Source = require('./Source'); -const { parseMd, renderMd, applyTemplates, clone } = require('./../parse_md'); +const md = require('../markdown'); const { spawnSync } = require('child_process'); const preprocessor = require('./preprocessor'); -const mdBuilder = require('./MDBuilder'); +const { MDOutline } = require('./MDBuilder'); +const missingDocs = require('./missingDocs'); -/** @typedef {import('./Documentation').MarkdownNode} MarkdownNode */ /** @typedef {import('./Documentation').Type} Type */ +/** @typedef {import('../markdown').MarkdownNode} MarkdownNode */ const PROJECT_DIR = path.join(__dirname, '..', '..'); const VERSION = require(path.join(PROJECT_DIR, 'package.json')).version; @@ -56,21 +57,20 @@ async function run() { let changedFiles = false; const header = fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-header.md')).toString(); - const body = fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-body.md')).toString(); const footer = fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-footer.md')).toString(); const links = fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-links.md')).toString(); - const params = fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-params.md')).toString(); - const apiSpec = applyTemplates(parseMd(body), parseMd(params)); + const outline = new MDOutline(path.join(PROJECT_DIR, 'docs-src', 'api-body.md'), path.join(PROJECT_DIR, 'docs-src', 'api-params.md')); // Produce api.md { const comment = ''; { - const { outline } = mdBuilder(apiSpec, false); const signatures = outline.signatures; + /** @type {MarkdownNode[]} */ const result = []; for (const clazz of outline.classesArray) { // Iterate over classes, create header node. + /** @type {MarkdownNode} */ const classNode = { type: 'h3', text: `class: ${clazz.name}` }; const match = clazz.name.match(/(JS|CDP|[A-Z])(.*)/); const varName = match[1].toLocaleLowerCase() + match[2]; @@ -81,10 +81,11 @@ async function run() { text: `[${clazz.name}]: #class-${clazz.name.toLowerCase()} "${clazz.name}"` }); // Append class comments - classNode.children = (clazz.spec || []).map(c => clone(c)); + classNode.children = (clazz.spec || []).map(c => md.clone(c)); for (const member of clazz.membersArray) { // Iterate members + /** @type {MarkdownNode} */ const memberNode = { type: 'h4', children: [] }; if (member.kind === 'event') { memberNode.text = `${varName}.on('${member.name}')`; @@ -112,7 +113,7 @@ async function run() { } // Append member doc - memberNode.children.push(...(member.spec || []).map(c => clone(c))); + memberNode.children.push(...(member.spec || []).map(c => md.clone(c))); classNode.children.push(memberNode); } } @@ -120,7 +121,7 @@ async function run() { type: 'text', text: links }); - api.setText([comment, header, renderMd(result, 10000), footer].join('\n')); + api.setText([comment, header, md.render(result), footer].join('\n')); } } @@ -139,8 +140,7 @@ async function run() { errors.push(`WARN: updated ${source.projectPath()}`); const jsSources = await Source.readdir(path.join(PROJECT_DIR, 'src', 'client'), '', []); - const missingDocs = require('./missingDocs.js'); - errors.push(...missingDocs(apiSpec, jsSources, path.join(PROJECT_DIR, 'src', 'client', 'api.ts'))); + errors.push(...missingDocs(outline, jsSources, path.join(PROJECT_DIR, 'src', 'client', 'api.ts'))); for (const source of mdSources) { if (!source.hasUpdatedText()) @@ -199,8 +199,9 @@ function renderProperty(name, type, spec) { if (type.properties && type.properties.length) children = type.properties.map(p => renderProperty(`\`${p.name}\``, p.type, p.spec)) else if (spec && spec.length > 1) - children = spec.slice(1).map(s => clone(s)); + children = spec.slice(1).map(s => md.clone(s)); + /** @type {MarkdownNode} */ const result = { type: 'li', liType: 'default', diff --git a/utils/doclint/generateApiJson.js b/utils/doclint/generateApiJson.js index 20d87fc7d9..6705e8644c 100644 --- a/utils/doclint/generateApiJson.js +++ b/utils/doclint/generateApiJson.js @@ -14,17 +14,14 @@ * limitations under the License. */ -const fs = require('fs'); +// @ts-check + const path = require('path'); -const { parseMd, applyTemplates } = require('../parse_md'); -const mdBuilder = require('./MDBuilder'); +const { MDOutline } = require('./MDBuilder'); const PROJECT_DIR = path.join(__dirname, '..', '..'); { - const apiBody = parseMd(fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-body.md')).toString()); - const apiParams = parseMd(fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-params.md')).toString()); - const api = applyTemplates(apiBody, apiParams); - const { documentation } = mdBuilder(api, false); + const { documentation } = new MDOutline(path.join(PROJECT_DIR, 'docs-src', 'api-body.md'), path.join(PROJECT_DIR, 'docs-src', 'api-params.md')); const result = serialize(documentation); console.log(JSON.stringify(result)); } diff --git a/utils/doclint/missingDocs.js b/utils/doclint/missingDocs.js index a6636c6cf3..37a905226f 100644 --- a/utils/doclint/missingDocs.js +++ b/utils/doclint/missingDocs.js @@ -15,18 +15,20 @@ * limitations under the License. */ -const mdBuilder = require('./MDBuilder'); const ts = require('typescript'); const EventEmitter = require('events'); const Documentation = require('./Documentation'); +/** @typedef {import('../../markdown').MarkdownNode} MarkdownNode */ + /** * @return {!Array} */ -module.exports = function lint(api, jsSources, apiFileName) { - const documentation = mdBuilder(api, true).documentation; - const apiMethods = listMethods(jsSources, apiFileName); +module.exports = function lint(outline, jsSources, apiFileName) { const errors = []; + const documentation = outline.documentation; + outline.copyDocsFromSuperclasses(errors); + const apiMethods = listMethods(jsSources, apiFileName); for (const [className, methods] of apiMethods) { const docClass = documentation.classes.get(className); if (!docClass) { @@ -75,11 +77,8 @@ module.exports = function lint(api, jsSources, apiFileName) { */ function paramsForMember(member) { if (member.kind !== 'method') - return []; - const paramNames = new Set(member.argsArray.map(a => a.name)); - if (member.options) - paramNames.add('options'); - return paramNames; + return new Set(); + return new Set(member.argsArray.map(a => a.name)); } /** @@ -124,10 +123,10 @@ function listMethods(sources, apiFileName) { methods = new Map(); apiMethods.set(className, methods); } - for (const [name, member] of classType.symbol.members || []) { + for (const [name, member] of /** @type {any[]} */(classType.symbol.members || [])) { if (name.startsWith('_') || name === 'T' || name === 'toString') continue; - if (EventEmitter.prototype.hasOwnProperty(name)) + if (/** @type {any} */(EventEmitter).prototype.hasOwnProperty(name)) continue; const memberType = checker.getTypeOfSymbolAtLocation(member, member.valueDeclaration); const signature = signatureForType(memberType); @@ -149,7 +148,7 @@ function listMethods(sources, apiFileName) { function visitMethods(node) { if (ts.isExportSpecifier(node)) { const className = node.name.text; - const exportSymbol = node.name ? checker.getSymbolAtLocation(node.name) : node.symbol; + const exportSymbol = node.name ? checker.getSymbolAtLocation(node.name) : /** @type {any} */ (node).symbol; const classType = checker.getDeclaredTypeOfSymbol(exportSymbol); if (!classType) throw new Error(`Cannot parse class "${className}"`); diff --git a/utils/doclint/test/missingDocs.spec.js b/utils/doclint/test/missingDocs.spec.js index e4a1fe706f..9a8e6a92a1 100644 --- a/utils/doclint/test/missingDocs.spec.js +++ b/utils/doclint/test/missingDocs.spec.js @@ -20,25 +20,25 @@ const path = require('path'); const missingDocs = require('../missingDocs'); const Source = require('../Source'); const { folio } = require('folio'); -const { parseMd } = require('../../parse_md'); +const { MDOutline } = require('../MDBuilder'); const { test, expect } = folio; test('missing docs', async ({}) => { - const api = parseMd(fs.readFileSync(path.join(__dirname, 'test-api.md')).toString()); + const outline = new MDOutline(path.join(__dirname, 'test-api.md')); const tsSources = [ await Source.readFile(path.join(__dirname, 'test-api.ts')), await Source.readFile(path.join(__dirname, 'test-api-class.ts')), ]; - const errors = missingDocs(api, tsSources, path.join(__dirname, 'test-api.ts')); + const errors = missingDocs(outline, tsSources, path.join(__dirname, 'test-api.ts')); expect(errors).toEqual([ 'Missing documentation for "Exists.exists2.extra"', 'Missing documentation for "Exists.exists2.options"', 'Missing documentation for "Exists.extra"', 'Missing documentation for "Extra"', + 'Documented "DoesNotExist" not found in sources', + 'Documented "Exists.doesNotExist" not found is sources', 'Documented "Exists.exists.doesNotExist" not found is sources', 'Documented "Exists.exists.options" not found is sources', - 'Documented "Exists.doesNotExist" not found is sources', - 'Documented "DoesNotExist" not found in sources', ]); }); diff --git a/utils/doclint/test/test-api.md b/utils/doclint/test/test-api.md index 4b607f88af..88d9ad699d 100644 --- a/utils/doclint/test/test-api.md +++ b/utils/doclint/test/test-api.md @@ -1,5 +1,11 @@ +# class: DoesNotExist + +## method: DoesNotExist.doesNotExist + # class: Exists +## method: Exists.doesNotExist + ## method: Exists.exists ### param: Exists.exists.exists @@ -12,9 +18,3 @@ - `option` <[number]> ## method: Exists.exists2 - -## method: Exists.doesNotExist - -# class: DoesNotExist - -## method: DoesNotExist.doesNotExist diff --git a/utils/generate_types/index.js b/utils/generate_types/index.js index 8cdfa5c501..368b8cd53d 100644 --- a/utils/generate_types/index.js +++ b/utils/generate_types/index.js @@ -22,7 +22,7 @@ const PROJECT_DIR = path.join(__dirname, '..', '..'); const fs = require('fs'); const {parseOverrides} = require('./parseOverrides'); const exported = require('./exported.json'); -const { parseMd, applyTemplates } = require('../parse_md'); +const { MDOutline } = require('../doclint/MDBuilder'); const objectDefinitions = []; const handledMethods = new Set(); @@ -36,11 +36,9 @@ let hadChanges = false; fs.mkdirSync(typesDir) writeFile(path.join(typesDir, 'protocol.d.ts'), fs.readFileSync(path.join(PROJECT_DIR, 'src', 'server', 'chromium', 'protocol.ts'), 'utf8')); writeFile(path.join(typesDir, 'trace.d.ts'), fs.readFileSync(path.join(PROJECT_DIR, 'src', 'trace', 'traceTypes.ts'), 'utf8')); - const apiBody = parseMd(fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-body.md')).toString()); - const apiParams = parseMd(fs.readFileSync(path.join(PROJECT_DIR, 'docs-src', 'api-params.md')).toString()); - const api = applyTemplates(apiBody, apiParams); - const mdResult = require('../doclint/MDBuilder')(api, true); - documentation = mdResult.documentation; + const outline = new MDOutline(path.join(PROJECT_DIR, 'docs-src', 'api-body.md'), path.join(PROJECT_DIR, 'docs-src', 'api-params.md')); + outline.copyDocsFromSuperclasses([]); + documentation = outline.documentation; // Root module types are overridden. const playwrightClass = documentation.classes.get('Playwright'); diff --git a/utils/parse_md.js b/utils/markdown.js similarity index 60% rename from utils/parse_md.js rename to utils/markdown.js index 0a1cae7698..e22589253e 100644 --- a/utils/parse_md.js +++ b/utils/markdown.js @@ -14,6 +14,17 @@ * limitations under the License. */ +// @ts-check + +/** @typedef {{ + * type: 'text' | 'li' | 'code' | 'gen' | 'h0' | 'h1' | 'h2' | 'h3' | 'h4', + * text?: string, + * codeLang?: string, + * lines?: string[], + * liType?: 'default' | 'bullet' | 'ordinal', + * children?: MarkdownNode[] + * }} MarkdownNode */ + function normalizeLines(content) { const inLines = content.replace(/\r\n/g, '\n').split('\n'); let inCodeBlock = false; @@ -47,19 +58,26 @@ function normalizeLines(content) { return outLines; } +/** + * @param {string[]} lines + */ function buildTree(lines) { + /** @type {MarkdownNode} */ const root = { type: 'h0', - value: '', + text: '', children: [] }; + /** @type {MarkdownNode[]} */ const stack = [root]; + /** @type {MarkdownNode[]} */ let liStack = null; for (let i = 0; i < lines.length; ++i) { let line = lines[i]; if (line.startsWith('```')) { + /** @type {MarkdownNode} */ const node = { type: 'code', lines: [], @@ -75,6 +93,7 @@ function buildTree(lines) { } if (line.startsWith('