🏗 Enforce JSDoc across build-system/ for complete type-checking (#3…

…4591)

.eslintrc.js

@@ -271,6 +271,7 @@ module.exports = {
         'extensions/**/test-e2e/*.js',
         'ads/**/test/**/*.js',
         'testing/**/*.js',
+        'build-system/**/test/*.js',
       ],
       'rules': {
         'require-jsdoc': 0,

build-system/.eslintrc.js

@@ -47,6 +47,5 @@ module.exports = {
     'local/no-module-exports': 0,
     'local/no-rest': 0,
     'local/no-spread': 0,
-    'require-jsdoc': 0,
   },
 };

build-system/OWNERS

@@ -10,5 +10,9 @@
       pattern: '{package.json,package-lock.json}',
       owners: [{name: 'ampproject/wg-infra', requestReviews: false}],
     },
+    {
+      pattern: 'tsconfig.json',
+      owners: [{name: 'rileyajones', notify: true}],
+    },
   ],
 }

build-system/babel-plugins/babel-plugin-transform-jss/index.js

@@ -61,6 +61,10 @@ module.exports = function ({template, types: t}) {
     return filename.endsWith('.jss.js');
   }
 
+  /**
+   * @param {string} name
+   * @return {string}
+   */
   function classnameId(name) {
     return `\$${name}`;
   }
@@ -238,6 +242,11 @@ module.exports = function ({template, types: t}) {
     );
   }
 
+  /**
+   * @param {Path} importDeclaration
+   * @param {string} name
+   * @return {string}
+   */
   function getImportIdentifier(importDeclaration, name) {
     return addNamed(
       importDeclaration,

build-system/common/esbuild-babel.js

@@ -44,6 +44,13 @@ function getEsbuildBabelPlugin(
     transformCache = new TransformCache('.babel-cache', '.js');
   }
 
+  /**
+   * @param {string} filename
+   * @param {string} contents
+   * @param {string} hash
+   * @param {Object} babelOptions
+   * @return {Promise}
+   */
   async function transformContents(filename, contents, hash, babelOptions) {
     if (enableCache) {
       const cached = transformCache.get(hash);

build-system/common/update-packages.js

@@ -201,7 +201,9 @@ function patchShadowDom() {
 
   writeIfUpdated(patchedName, file);
 }
-
+/**
+ * Adds a missing export statement to the preact module.
+ */
 function patchPreact() {
   fs.ensureDirSync('node_modules/preact/dom');
   const file = `export { render, hydrate } from 'preact';`;

build-system/compile/compile.js

@@ -79,6 +79,9 @@ async function closureCompile(
   // Rate limit closure compilation to MAX_PARALLEL_CLOSURE_INVOCATIONS
   // concurrent processes.
   return new Promise(function (resolve, reject) {
+    /**
+     * Kicks off the first closure invocation.
+     */
     function start() {
       inProgress++;
       compile(
@@ -97,6 +100,9 @@ async function closureCompile(
       );
     }
 
+    /**
+     * Keeps track of the invocation count.
+     */
     function next() {
       if (!queue.length) {
         return;
@@ -110,6 +116,9 @@ async function closureCompile(
   });
 }
 
+/**
+ * Cleans up the placeholder directories for fake build modules.
+ */
 function cleanupBuildDir() {
   del.sync('build/fake-module');
   del.sync('build/patched-module');
@@ -441,6 +450,9 @@ async function compile(
   }
 }
 
+/**
+ * Indicates the current closure concurrency and how to override it.
+ */
 function printClosureConcurrency() {
   log(
     green('Using up to'),

build-system/compile/helpers.js

@@ -21,10 +21,15 @@ const path = require('path');
 const {getBabelOutputDir} = require('./pre-closure-babel');
 const {VERSION: internalRuntimeVersion} = require('./internal-version');
 
+/**
+ * Computes the base url for sourcemaps. Custom sourcemap URLs have placeholder
+ * {version} that should be replaced with the actual version. Also, ensures
+ * that a trailing slash exists.
+ * @param {Object} options
+ * @return {string}
+ */
 function getSourceMapBase(options) {
   if (argv.sourcemap_url) {
-    // Custom sourcemap URLs have placeholder {version} that should be
-    // replaced with the actual version. Also, ensure trailing slash exists.
     return String(argv.sourcemap_url)
       .replace(/\{version\}/g, internalRuntimeVersion)
       .replace(/([^/])$/, '$1/');
@@ -35,6 +40,10 @@ function getSourceMapBase(options) {
   return `https://raw.githubusercontent.com/ampproject/amphtml/${internalRuntimeVersion}/`;
 }
 
+/**
+ * Updates all filepaths in the sourcemap output.
+ * @param {Object} sourcemaps
+ */
 function updatePaths(sourcemaps) {
   const babelOutputDir = getBabelOutputDir();
   sourcemaps.sources = sourcemaps.sources.map((source) =>
@@ -47,6 +56,12 @@ function updatePaths(sourcemaps) {
   }
 }
 
+/**
+ * Writes the sourcemap output to disk.
+ * @param {string} sourcemapsFile
+ * @param {Object} options
+ * @return {Promise<void>}
+ */
 async function writeSourcemaps(sourcemapsFile, options) {
   const sourcemaps = await fs.readJson(sourcemapsFile);
   updatePaths(sourcemaps);

build-system/compile/sanitize.js

@@ -19,16 +19,22 @@ const argv = require('minimist')(process.argv.slice(2));
 const fs = require('fs-extra');
 const prettier = require('prettier');
 
+/**
+ * Sanitizes variable names in minified output to aid in debugging.
+ * 1. Normalizes the length of all jscomp variables, so that prettier will
+ *    format it the same.
+ * 2. Strips numbers from the sanitized jscomp variables so that a single extra
+ *    variable doesn't cause thousands of diffs.
+ * @param {string} file
+ * @return {Promise<void>}
+ */
 async function sanitize(file) {
   if (!argv.sanitize_vars_for_diff) {
     return;
   }
-
   const contents = await fs.readFile(file, 'utf-8');
   const config = await prettier.resolveConfig(file);
   const options = {filepath: file, parser: 'babel', ...config};
-  // Normalize the length of all jscomp variables, so that prettier will
-  // format it the same.
   const replaced = Object.create(null);
   let count = 0;
   const presanitize = contents.replace(
@@ -38,8 +44,6 @@ async function sanitize(file) {
       (replaced[match] = `___${String(count++).padStart(6, '0')}___`)
   );
   const formatted = prettier.format(presanitize, options);
-  // Finally, strip the numbers from the sanitized jscomp variables. This
-  // is so that a single extra variable doesn't cause thousands of diffs.
   const sanitized = formatted.replace(/___\d+___/g, '______');
   await fs.outputFile(file, sanitized);
 }

build-system/eslint-rules/forbidden-terms-config.js

@@ -31,6 +31,10 @@ module.exports = {
     // greatly speeds up this rule.
     const fileContent = {};
 
+    /**
+     * @param {string} filename
+     * @return {string}
+     */
     function readFileContent(filename) {
       try {
         return (
@@ -42,6 +46,10 @@ module.exports = {
       }
     }
 
+    /**
+     * @param {Object} fixer
+     * @param {Node} node
+     */
     function* removeFromArray(fixer, node) {
       const {text} = context.getSourceCode();
       let {start} = node;

build-system/pr-check/bundle-size.js

@@ -25,12 +25,18 @@ const {Targets, buildTargetsInclude} = require('./build-targets');
 
 const jobName = 'bundle-size.js';
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   timedExecOrDie('amp dist --noconfig --esm');
   timedExecOrDie('amp dist --noconfig');
   timedExecOrDie('amp bundle-size --on_push_build');
 }
 
+/**
+ * Steps to run during PR builds.
+ */
 function prBuildWorkflow() {
   if (buildTargetsInclude(Targets.RUNTIME)) {
     timedExecOrDie('amp dist --noconfig --esm');

build-system/pr-check/checks.js

@@ -26,6 +26,9 @@ const {timedExecOrDie} = require('./utils');
 
 const jobName = 'checks.js';
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   timedExecOrDie('amp presubmit');
   timedExecOrDie('amp check-invalid-whitespaces');
@@ -53,6 +56,7 @@ function pushBuildWorkflow() {
 }
 
 /**
+ * Steps to run during PR builds.
  * @return {Promise<void>}
  */
 async function prBuildWorkflow() {

build-system/pr-check/cross-browser-tests.js

@@ -98,14 +98,17 @@ function runUnitTestsForPlatform() {
       );
   }
 }
-
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   runUnitTestsForPlatform();
   timedExecOrDie('amp dist --fortesting');
   runIntegrationTestsForPlatform();
 }
 
 /**
+ * Steps to run during PR builds.
  * @return {Promise<void>}
  */
 async function prBuildWorkflow() {

build-system/pr-check/e2e-tests.js

@@ -29,6 +29,9 @@ const {Targets, buildTargetsInclude} = require('./build-targets');
 
 const jobName = 'e2e-tests.js';
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   try {
     timedExecOrThrow(
@@ -44,6 +47,9 @@ function pushBuildWorkflow() {
   }
 }
 
+/**
+ * Steps to run during PR builds.
+ */
 function prBuildWorkflow() {
   if (buildTargetsInclude(Targets.RUNTIME, Targets.E2E_TEST)) {
     timedExecOrDie('amp e2e --nobuild --headless --compiled');

build-system/pr-check/experiment-build.js

@@ -31,6 +31,9 @@ const {Targets, buildTargetsInclude} = require('./build-targets');
 
 const jobName = `${experiment}-build.js`;
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   // Note that if config is invalid, this build would have been skipped by CircleCI.
   const config = getExperimentConfig(experiment);
@@ -39,6 +42,9 @@ function pushBuildWorkflow() {
   storeExperimentBuildToWorkspace(experiment);
 }
 
+/**
+ * Steps to run during PR builds.
+ */
 function prBuildWorkflow() {
   if (
     buildTargetsInclude(

build-system/pr-check/experiment-tests.js

@@ -58,12 +58,18 @@ function runExperimentTests(config) {
   }
 }
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   // Note that if config is invalid, this build would have been skipped by CircleCI.
   const config = getExperimentConfig(experiment);
   runExperimentTests(config);
 }
 
+/**
+ * Steps to run during PR builds.
+ */
 function prBuildWorkflow() {
   if (
     buildTargetsInclude(

build-system/pr-check/module-build.js

@@ -29,11 +29,17 @@ const {Targets, buildTargetsInclude} = require('./build-targets');
 
 const jobName = 'module-build.js';
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   timedExecOrDie('amp dist --esm --fortesting');
   storeModuleBuildToWorkspace();
 }
 
+/**
+ * Steps to run during PR builds.
+ */
 function prBuildWorkflow() {
   // TODO(#31102): This list must eventually match the same buildTargets check
   // found in pr-check/nomodule-build.js as we turn on the systems that

build-system/pr-check/module-tests.js

@@ -27,6 +27,9 @@ const {Targets, buildTargetsInclude} = require('./build-targets');
 
 const jobName = 'module-tests.js';
 
+/**
+ * Adds a canary or prod config string to all esm and non-esm minified targets.
+ */
 function prependConfig() {
   const targets = MINIFIED_TARGETS.flatMap((target) => [
     `dist/${target}.js`,
@@ -37,11 +40,17 @@ function prependConfig() {
   );
 }
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   prependConfig();
   timedExecOrDie('amp integration --nobuild --compiled --headless --esm');
 }
 
+/**
+ * Steps to run during PR builds.
+ */
 function prBuildWorkflow() {
   if (buildTargetsInclude(Targets.RUNTIME, Targets.INTEGRATION_TEST)) {
     prependConfig();

build-system/pr-check/nomodule-build.js

@@ -37,12 +37,16 @@ const {Targets, buildTargetsInclude} = require('./build-targets');
 
 const jobName = 'nomodule-build.js';
 
+/**
+ * Steps to run during push builds.
+ */
 function pushBuildWorkflow() {
   timedExecOrDie('amp dist --fortesting');
   storeNomoduleBuildToWorkspace();
 }
 
 /**
+ * Steps to run during PR builds.
  * @return {Promise<void>}
  */
 async function prBuildWorkflow() {