Types: fix all type parse errors and ensure no new ones crop up (ampp…

…roject#34105) * Types: fix all type parse errors * self nits * run prettier * fix a new parse error, make low-bar target
rochapablo · Aug 30, 2021 · 1dd82bf · 1dd82bf
1 parent 42b9030
commit 1dd82bf
Show file tree

Hide file tree

Showing 26 changed files with 133 additions and 70 deletions.
diff --git a/ads/vendors/feedad.js b/ads/vendors/feedad.js
@@ -57,7 +57,7 @@ import {setStyle} from '../../src/style';
  */
 
 /**
- * @param {!FeedAdGlobal & Window} global
+ * @param {!FeedAdGlobal} global
  * @param {!FeedAdData} data
  */
 export function feedad(global, data) {

diff --git a/build-system/compile/closure-compile.js b/build-system/compile/closure-compile.js
@@ -52,7 +52,9 @@ function logClosureCompilerError(message) {
  */
 function handleClosureCompilerError(err, outputFilename, options) {
   if (options.typeCheckOnly) {
-    log(`${red('ERROR:')} Type checking failed`);
+    if (!options.logger) {
+      log(`${red('ERROR:')} Type checking failed`);
+    }
     return err;
   }
   log(`${red('ERROR:')} Could not minify ${cyan(outputFilename)}`);
@@ -69,10 +71,14 @@ function handleClosureCompilerError(err, outputFilename, options) {
  * on Windows exceeds the command line size limit. The stream mode is 'IN'
  * because output files and sourcemaps are written directly to disk.
  * @param {Array<string>} flags
+ * @param {!Object} options
  * @return {!Object}
  */
-function initializeClosure(flags) {
-  const pluginOptions = {streamMode: 'IN', logger: logClosureCompilerError};
+function initializeClosure(flags, options) {
+  const pluginOptions = {
+    streamMode: 'IN',
+    logger: options.logger || logClosureCompilerError,
+  };
   return compiler.gulp()(flags, pluginOptions);
 }
 
@@ -88,7 +94,7 @@ function runClosure(outputFilename, options, flags, srcFiles) {
   return new Promise((resolve, reject) => {
     vinylFs
       .src(srcFiles, {base: getBabelOutputDir()})
-      .pipe(initializeClosure(flags))
+      .pipe(initializeClosure(flags, options))
       .on('error', (err) => {
         const reason = handleClosureCompilerError(err, outputFilename, options);
         reason ? reject(reason) : resolve();
@@ -99,5 +105,6 @@ function runClosure(outputFilename, options, flags, srcFiles) {
 }
 
 module.exports = {
+  logClosureCompilerError,
   runClosure,
 };
diff --git a/build-system/tasks/check-types.js b/build-system/tasks/check-types.js
@@ -28,6 +28,7 @@ const {compileCss} = require('./css');
 const {compileJison} = require('./compile-jison');
 const {cyan, green, yellow, red} = require('kleur/colors');
 const {extensions, maybeInitializeExtensions} = require('./extension-helpers');
+const {logClosureCompilerError} = require('../compile/closure-compile');
 const {log} = require('../common/logging');
 const {typecheckNewServer} = require('../server/typescript-compile');
 
@@ -159,6 +160,33 @@ const TYPE_CHECK_TARGETS = {
     externGlobs: [CORE_EXTERNS_GLOB, 'build-system/externs/*.extern.js'],
   },
 
+  /*
+   * Ensures that all files in src and extensions pass the specified set of errors.
+   */
+  'low-bar': {
+    entryPoints: ['src/amp.js'],
+    extraGlobs: ['{src,extensions}/**/*.js'],
+    onError(msg) {
+      const lowBarErrors = [
+        'JSC_BAD_JSDOC_ANNOTATION',
+        'JSC_INVALID_PARAM',
+        'JSC_TYPE_PARSE_ERROR',
+      ];
+      const lowBarRegex = new RegExp(lowBarErrors.join('|'));
+
+      const targetErrors = msg
+        .split('\n')
+        .filter((s) => lowBarRegex.test(s))
+        .join('\n')
+        .trim();
+
+      if (targetErrors.length) {
+        logClosureCompilerError(targetErrors);
+        throw new Error(`Type-checking failed for target ${cyan('low-bar')}`);
+      }
+    },
+  },
+
   // TODO(#33631): Targets below this point are not expected to pass.
   // They can possibly be removed?
   'src': {
@@ -246,12 +274,19 @@ async function typeCheck(targetName) {
     return;
   }
 
+  let errorMsg;
   await closureCompile(entryPoints, './dist', `${targetName}-check-types.js`, {
     noAddDeps,
     include3pDirectories: !noAddDeps,
     includePolyfills: !noAddDeps,
     typeCheckOnly: true,
+    logger: (m) => (errorMsg = m),
     ...opts,
+  }).catch((error) => {
+    if (!target.onError) {
+      throw error;
+    }
+    target.onError(errorMsg);
   });
   log(green('SUCCESS:'), 'Type-checking passed for target', cyan(targetName));
 }

diff --git a/extensions/amp-a4a/0.1/amp-ad-utils.js b/extensions/amp-a4a/0.1/amp-ad-utils.js
@@ -167,7 +167,7 @@ export function getAmpAdMetadata(creative) {
 /**
  * Merges any elements from customElementExtensions array into extensions array if
  * the element is not present.
- * @param {!Array<{custom-element: string, 'src': string}} extensions
+ * @param {!Array<{custom-element: string, 'src': string}>} extensions
  * @param {!Array<string>} customElementExtensions
  */
 export function mergeExtensionsMetadata(extensions, customElementExtensions) {

diff --git a/extensions/amp-accordion/1.0/component.type.js b/extensions/amp-accordion/1.0/component.type.js
@@ -57,7 +57,7 @@ AccordionDef.AccordionHeaderProps;
 /**
  * @typedef {{
  *   as: (string|PreactDef.FunctionalComponent|undefined),
- *   role: (string|undefined)
+ *   role: (string|undefined),
  *   className: (string|undefined),
  *   id: (string|undefined),
  *   children: (?PreactDef.Renderable|undefined),

diff --git a/extensions/amp-addthis/0.1/amp-addthis.js b/extensions/amp-addthis/0.1/amp-addthis.js
@@ -403,13 +403,18 @@ class AmpAddThis extends AMP.BaseElement {
     }, {});
   }
 
+  /**
+   * @typedef {{
+   *   ampdoc: !../../../src/service/ampdoc-impl.AmpDoc,
+   *   loc: *,
+   *   pubId: *,
+   * }} SetupListenersInput
+   */
+
   /**
    * Sets up listeners.
    *
-   * @param {!Object} input
-   * @param {!../../../src/service/ampdoc-impl.AmpDoc} [input.ampdoc]
-   * @param {*} [input.loc]
-   * @param {*} [input.pubId]
+   * @param {!SetupListenersInput} input
    * @memberof AmpAddThis
    */
   setupListeners_(input) {

diff --git a/extensions/amp-addthis/0.1/config-manager.js b/extensions/amp-addthis/0.1/config-manager.js
@@ -111,14 +111,19 @@ export class ConfigManager {
   }
 
   /**
-   * @param {!Object} input
-   * @param {*} [input.iframe]
-   * @param {string} [input.widgetId]
-   * @param {string} [input.pubId]
-   * @param {!Object} [input.shareConfig]
-   * @param {!Object} [input.atConfig]
-   * @param {string} [input.productCode]
-   * @param {string} [input.containerClassName]
+   * @typedef {{
+   *  iframe: *,
+   *  widgetId: string,
+   *  pubId: string,
+   *  shareConfig: !Object,
+   *  atConfig: !Object,
+   *  productCode: string,
+   *  containerClassName: string,
+   * }} SendConfigurationInput
+   */
+
+  /**
+   * @param {!SendConfigurationInput} input
    * @private
    */
   sendConfiguration_(input) {

diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js
@@ -97,6 +97,15 @@ let BoundPropertyDef;
  */
 let BoundElementDef;
 
+/**
+ * The options bag for binding application.
+ *
+ * @typedef {Record} ApplyOptionsDef
+ * @property {boolean=} skipAmpState If true, skips <amp-state> elements.
+ * @property {Array<!Element>=} constrain If provided, restricts application to children of the provided elements.
+ * @property {boolean=} evaluateOnly If provided, caches the evaluated result on each bound element and skips the actual DOM updates.
+ */
+
 /**
  * A map of tag names to arrays of attributes that do not have non-bind
  * counterparts. For instance, amp-carousel allows a `[slide]` attribute,
@@ -1188,12 +1197,7 @@ export class Bind {
    * Applies expression results to elements in the document.
    *
    * @param {Object<string, BindExpressionResultDef>} results
-   * @param {!Object} opts
-   * @param {boolean=} opts.skipAmpState If true, skips <amp-state> elements.
-   * @param {Array<!Element>=} opts.constrain If provided, restricts application
-   *   to children of the provided elements.
-   * @param {boolean=} opts.evaluateOnly If provided, caches the evaluated
-   *   result on each bound element and skips the actual DOM updates.
+   * @param {!ApplyOptionsDef} opts
    * @return {!Promise}
    * @private
    */

diff --git a/extensions/amp-brightcove/0.1/amp-brightcove.js b/extensions/amp-brightcove/0.1/amp-brightcove.js
@@ -142,7 +142,7 @@ class AmpBrightcove extends AMP.BaseElement {
   }
 
   /**
-   * @return {Promise[]}
+   * @return {Promise}
    */
   getConsents_() {
     const consentPolicy = super.getConsentPolicy();

diff --git a/extensions/amp-lightbox-gallery/1.0/component.type.js b/extensions/amp-lightbox-gallery/1.0/component.type.js
@@ -32,7 +32,7 @@ LightboxGalleryDef.Props;
  *   as: (string|undefined),
  *   children: (!PreactDef.Renderable),
  *   enableActivation: (boolean|undefined),
- *   onClick: (function(Event)|undefined)
+ *   onClick: (function(Event)|undefined),
  *   render: (function():PreactDef.Renderable),
  * }}
  */
@@ -42,7 +42,7 @@ LightboxGalleryDef.WithLightboxProps;
  * @typedef {{
  *   deregister: (function(string):undefined),
  *   register: (function(string, Element):undefined),
- *   open: (function:undefined),
+ *   open: (function():undefined),
  * }}
  */
 LightboxGalleryDef.ContextProps;
diff --git a/extensions/amp-lightbox/1.0/component.type.js b/extensions/amp-lightbox/1.0/component.type.js
@@ -23,27 +23,27 @@ var LightboxDef = {};
  *   id: (string),
  *   animation: (string|undefined),
  *   children: (?PreactDef.Renderable|undefined),
- *   closeButtonAs: (function:PreactDef.Renderable|undefined),
+ *   closeButtonAs: (function():PreactDef.Renderable|undefined),
  *   scrollable: (boolean),
  *   initialOpen: (boolean),
- *   onBeforeOpen: (function|undefined),
- *   onAfterOpen: (function|undefined),
- *   onAfterClose: (function|undefined),
+ *   onBeforeOpen: (function():void|undefined),
+ *   onAfterOpen: (function():void|undefined),
+ *   onAfterClose: (function():void|undefined),
  * }}
  */
 LightboxDef.Props;
 
 /**
  * @typedef {{
  *   aria-label: (string),
- *   as: (function:PreactDef.Renderable|undefined),
- *   onClick: function,
+ *   as: (function():PreactDef.Renderable|undefined),
+ *   onClick: function():void,
  * }}
  */
 LightboxDef.CloseButtonProps;
 
 /** @interface */
-Lightbox.LightboxApi = class {
+LightboxDef.LightboxApi = class {
   /** Open the lightbox */
   open() {}
 

diff --git a/extensions/amp-render/1.0/component.type.js b/extensions/amp-render/1.0/component.type.js
@@ -22,7 +22,7 @@ var RenderDef = {};
 /**
  * @typedef {{
  *   src: (!string),
- *   getJson: (!Function)
+ *   getJson: (!Function),
  *   render: (?RendererFunctionType|undefined),
  * }}
  */

diff --git a/extensions/amp-selector/1.0/component.type.js b/extensions/amp-selector/1.0/component.type.js
@@ -56,7 +56,7 @@ SelectorDef.OptionProps;
 /**
  * @typedef {{
  *   disabled: (boolean|undefined),
- *   focusRef: ({current: {active: *, focusMap: !Object}),
+ *   focusRef: ({current: {active: *, focusMap: !Object}}),
  *   keyboardSelectMode: (string|undefined),
  *   multiple: (boolean|undefined),
  *   optionsRef: ({current: !Array<*>}),

diff --git a/extensions/amp-sidebar/1.0/component.type.js b/extensions/amp-sidebar/1.0/component.type.js
@@ -23,9 +23,9 @@ var SidebarDef = {};
  * @typedef {{
  *   as: (string|PreactDef.FunctionalComponent|undefined),
  *   side: (string|undefined),
- *   onBeforeOpen: (function|undefined),
- *   onAfterOpen: (function|undefined),
- *   onAfterClose: (function|undefined),
+ *   onBeforeOpen: (function():void|undefined),
+ *   onAfterOpen: (function():void|undefined),
+ *   onAfterClose: (function():void|undefined),
  *   backdropStyle: (?Object|undefined),
  *   backdropClassName: (string|undefined),
  *   children: (?PreactDef.Renderable|undefined),

diff --git a/extensions/amp-sidebar/1.0/sidebar-animations-hook.js b/extensions/amp-sidebar/1.0/sidebar-animations-hook.js
@@ -49,12 +49,12 @@ function safelySetStyles(element, styles) {
 /**
  * @param {boolean} mounted
  * @param {boolean} opened
- * @param {{current: function|undefined}} onAfterOpen
- * @param {{current: function|undefined}} onAfterClose
+ * @param {{current: function():void} | {current: void}} onAfterOpen
+ * @param {{current: function():void} | {current: void}} onAfterClose
  * @param {string} side
- * @param {{current: Element|null}} sidebarRef
- * @param {{current: Element|null}} backdropRef
- * @param {function} setMounted
+ * @param {{current: (Element|null)}} sidebarRef
+ * @param {{current: (Element|null)}} backdropRef
+ * @param {function():undefined} setMounted
  */
 export function useSidebarAnimation(
   mounted,

diff --git a/extensions/amp-story-auto-ads/0.1/story-ad-page-manager.js b/extensions/amp-story-auto-ads/0.1/story-ad-page-manager.js
@@ -69,7 +69,7 @@ export class StoryAdPageManager {
     /** @private {!./story-ad-button-text-fitter.ButtonTextFitter} */
     this.buttonFitter_ = new ButtonTextFitter(this.ampdoc_);
 
-    /** @private {Object<string, StoryAdPage} */
+    /** @private {Object<string, StoryAdPage>} */
     this.pages_ = {};
 
     /** @private {!../../amp-story/1.0/amp-story-store-service.AmpStoryStoreService} **/

diff --git a/extensions/amp-story-auto-ads/0.1/story-ad-ui.js b/extensions/amp-story-auto-ads/0.1/story-ad-ui.js
@@ -111,7 +111,7 @@ export function getStoryAdMetadataFromElement(adElement) {
 /**
  * Returns a boolean indicating if there is sufficent metadata to render CTA.
  * @param {!StoryAdUIMetadata} metadata
- * @param {=boolean} opt_inabox
+ * @param {boolean=} opt_inabox
  * @return {boolean}
  */
 export function validateCtaMetadata(metadata, opt_inabox) {

diff --git a/extensions/amp-story-dev-tools/0.1/amp-story-dev-tools-tab-preview.js b/extensions/amp-story-dev-tools/0.1/amp-story-dev-tools-tab-preview.js
@@ -229,18 +229,18 @@ const MAX_DEVICE_SPACES = 4;
 /**
  * @typedef {{
  *  element: !Element,
- *  player: !Element
+ *  player: !Element,
  *  chip: !Element,
  *  width: number,
  *  height: number,
  *  deviceHeight: ?number,
  *  deviceSpaces: number,
- * }}
+ * }} DeviceInfo
+ *
  * Contains the data related to the device.
  * Width and height refer to the story viewport, while deviceHeight is the device screen height.
  * The deviceSpaces refers to the MAX_DEVICE_SPACES, ensuring the devices on screen don't go over the max space set.
  */
-export let DeviceInfo;
 
 const DEFAULT_DEVICES = 'iphone11native;oneplus5t;pixel2';
 
@@ -363,7 +363,7 @@ function simplifyDeviceName(name) {
  * Eg: `devices="ipad;iphone"` will find the ipad and also the first device in ALL_DEVICES
  * that starts with "iphone" (ignoring case and symbols).
  * @param {string} queryHash
- * @return {any[]}
+ * @return {Array<*>}
  */
 function parseDevices(queryHash) {
   const screenSizes = [];