From f38ca1fa3a0335a9444b8cdeb7bf45bd2883a7bd Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Thu, 29 Apr 2021 15:21:33 -0400 Subject: [PATCH 1/4] Types: fix all type parse errors --- build-system/compile/closure-compile.js | 15 +++++-- build-system/tasks/check-types.js | 40 +++++++++++++++++++ extensions/amp-a4a/0.1/amp-ad-utils.js | 2 +- .../amp-accordion/1.0/component.type.js | 2 +- extensions/amp-addthis/0.1/amp-addthis.js | 13 ++++-- extensions/amp-addthis/0.1/config-manager.js | 21 ++++++---- extensions/amp-bind/0.1/bind-impl.js | 19 ++++++--- .../amp-brightcove/0.1/amp-brightcove.js | 2 +- .../1.0/component.type.js | 4 +- extensions/amp-lightbox/1.0/component.type.js | 14 +++---- extensions/amp-render/1.0/component.type.js | 2 +- extensions/amp-selector/1.0/component.type.js | 2 +- extensions/amp-sidebar/1.0/component.type.js | 6 +-- .../1.0/sidebar-animations-hook.js | 10 ++--- .../0.1/story-ad-page-manager.js | 2 +- .../amp-story-auto-ads/0.1/story-ad-ui.js | 2 +- .../0.1/amp-story-dev-tools-tab-preview.js | 8 ++-- .../amp-subscriptions/0.1/entitlement.js | 23 +++++++---- .../amp-story-player-viewport-observer.js | 6 +-- src/context/node.js | 2 +- src/custom-element.js | 6 +-- src/preact/component/3p-frame.js | 2 +- src/preact/component/component.type.js | 2 +- src/preact/slot.js | 2 +- 24 files changed, 139 insertions(+), 68 deletions(-) diff --git a/build-system/compile/closure-compile.js b/build-system/compile/closure-compile.js index 63e86d86751c..ad25f5de00fc 100644 --- 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} 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 index 6f8376682fd3..ba2fec0ca788 100644 --- 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'); @@ -256,6 +257,44 @@ async function typeCheck(targetName) { log(green('SUCCESS:'), 'Type-checking passed for target', cyan(targetName)); } +/** + * Typechecks all JS files within src and extensions for a specific set + * of closure errors. + * + * This is a temporary measure while we restore 100% typechecking. + * + * @param {Array} entryPoints + */ +async function ensureMinTypeQuality(entryPoints) { + const srcGlobs = ['src/**/*.js', 'extensions/**/*.js']; + const errorsToCheckFor = [ + 'JSC_BAD_JSDOC_ANNOTATION', + 'JSC_INVALID_PARAM', + 'JSC_TYPE_PARSE_ERROR', + ]; + + let msg = ''; + await closureCompile(entryPoints, './dist', `check-types.js`, { + include3pDirectories: false, + includePolyfills: false, + typeCheckOnly: true, + extraGlobs: globby.sync(srcGlobs), + warningLevel: 'VERBOSE', + logger: (str) => (msg = str), + }).catch(() => {}); + + const targetErrors = msg + .split('\n') + .filter((s) => errorsToCheckFor.some((error) => s.includes(error))) + .join('\n') + .trim(); + + if (targetErrors.length) { + logClosureCompilerError(targetErrors); + throw new Error('Found parse errors'); + } +} + /** * Runs closure compiler's type checker against all AMP code. * @return {!Promise} @@ -278,6 +317,7 @@ async function checkTypes() { log(`Checking types for targets: ${targets.map(cyan).join(', ')}`); displayLifecycleDebugging(); + await ensureMinTypeQuality(['src/amp.js']); await Promise.all(targets.map(typeCheck)); exitCtrlcHandler(handlerProcess); } diff --git a/extensions/amp-a4a/0.1/amp-ad-utils.js b/extensions/amp-a4a/0.1/amp-ad-utils.js index d820ec1aed89..4bcd713dc016 100644 --- 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} 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 index 556b7cb4a0a5..6ffcbad63204 100644 --- 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 index 3b512458941a..bd16427fc22c 100644 --- 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 index 37f44d950e55..d1e6d559bab5 100644 --- 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 index b868b9a1b514..81a8ec8d9fe0 100644 --- a/extensions/amp-bind/0.1/bind-impl.js +++ b/extensions/amp-bind/0.1/bind-impl.js @@ -97,6 +97,18 @@ let BoundPropertyDef; */ let BoundElementDef; +/** + * The options bag for binding application. + * - skipAmpState: If true, skips elements. + * - constrain: If provided, restricts application to children of the provided elements. + * - evaluateOnly: If provided, caches the evaluated result on each bound element and skips the actual DOM updates. + * + * @typedef {Record} ApplyOptionsDef + * @property {boolean=} skipAmpState - If true, skips elements. + * @property {Array=} constrain + * @property {boolean=} evaluateOnly + */ + /** * 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 +1200,7 @@ export class Bind { * Applies expression results to elements in the document. * * @param {Object} results - * @param {!Object} opts - * @param {boolean=} opts.skipAmpState If true, skips elements. - * @param {Array=} 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 index a1369100c5b4..20218b8a3649 100644 --- 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 index b420a1402f6d..23c0e0ac3821 100644 --- 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 index 1d63c6ec5a9c..36704894887c 100644 --- a/extensions/amp-lightbox/1.0/component.type.js +++ b/extensions/amp-lightbox/1.0/component.type.js @@ -23,12 +23,12 @@ 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; @@ -36,14 +36,14 @@ 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 index 007fc0520444..c13e46ff7e9b 100644 --- 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 index 2848e03cdc52..496ba4f78b8e 100644 --- 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 index 3f3e49cc3f5e..e6ac472b45e6 100644 --- 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 index 6a6085452d47..780e16f23400 100644 --- 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 index e9df1e4584de..2aa66c848b87 100644 --- 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} */ 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 index 3170087b7194..4eb597ac9088 100644 --- 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 index defb7e86bec4..49b86e475379 100644 --- 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 = []; diff --git a/extensions/amp-subscriptions/0.1/entitlement.js b/extensions/amp-subscriptions/0.1/entitlement.js index db4e30d9a2aa..543e8a8b3b6f 100644 --- a/extensions/amp-subscriptions/0.1/entitlement.js +++ b/extensions/amp-subscriptions/0.1/entitlement.js @@ -23,6 +23,20 @@ export const GrantReason = { 'LAA': 'LAA', }; +/** + * The constructor arg for an {@link Entitlement} + * + * @typedef {{ + * source: string, + * raw: string, + * service: string, + * granted: boolean, + * grantReason: ?GrantReason, + * dataObject: ?JsonObject, + * decryptedDocumentKey: ?string + * }} EntitlementConstructorInputDef + */ + /** * The single entitlement object. */ @@ -41,14 +55,7 @@ export class Entitlement { } /** - * @param {Object} input - * @param {string} [input.source] - * @param {string} [input.raw] - * @param {string} [input.service] - * @param {boolean} [input.granted] - * @param {?GrantReason} [input.grantReason] - * @param {?JsonObject} [input.dataObject] - * @param {?string} [input.decryptedDocumentKey] + * @param {!EntitlementConstructorInputDef} input */ constructor(input) { const { diff --git a/src/amp-story-player/amp-story-player-viewport-observer.js b/src/amp-story-player/amp-story-player-viewport-observer.js index b9df900cf643..4713a733ff52 100644 --- a/src/amp-story-player/amp-story-player-viewport-observer.js +++ b/src/amp-story-player/amp-story-player-viewport-observer.js @@ -27,7 +27,7 @@ export class AmpStoryPlayerViewportObserver { /** * @param {!Window} win * @param {!Element} element - * @param {function} viewportCb + * @param {function():void} viewportCb */ constructor(win, element, viewportCb) { /** @private {!Window} */ @@ -36,10 +36,10 @@ export class AmpStoryPlayerViewportObserver { /** @private {!Element} */ this.element_ = element; - /** @private {function} */ + /** @private {function():void} */ this.cb_ = viewportCb; - /** @private {?function} */ + /** @private {?function():void} */ this.scrollHandler_ = null; this.initializeInObOrFallback_(); diff --git a/src/context/node.js b/src/context/node.js index 151712a20670..cff817acd8b5 100644 --- a/src/context/node.js +++ b/src/context/node.js @@ -396,7 +396,7 @@ export class ContextNode { * of factory is important to reduce bundling costs for context node. * * @param {*} id - * @param {fucntion(new:./subscriber.Subscriber, function(...?), !Array)} constr + * @param {function(new:./subscriber.Subscriber, function(...?), !Array):void} constr * @param {!Function} func * @param {!Array} deps */ diff --git a/src/custom-element.js b/src/custom-element.js index 8e1f5963905b..0762dccf7f8a 100644 --- a/src/custom-element.js +++ b/src/custom-element.js @@ -89,7 +89,7 @@ function isTemplateTagSupported() { * Creates a named custom element class. * * @param {!Window} win The window in which to register the custom element. - * @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement element, ?(typeof BaseElement))} elementConnectedCallback + * @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement, ?(typeof BaseElement))} elementConnectedCallback * @return {typeof AmpElement} The custom element class. */ export function createCustomElementClass(win, elementConnectedCallback) { @@ -107,7 +107,7 @@ export function createCustomElementClass(win, elementConnectedCallback) { * Creates a base custom element class. * * @param {!Window} win The window in which to register the custom element. - * @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement element, ?(typeof BaseElement))} elementConnectedCallback + * @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement, ?(typeof BaseElement))} elementConnectedCallback * @return {typeof HTMLElement} */ function createBaseCustomElementClass(win, elementConnectedCallback) { @@ -2177,7 +2177,7 @@ function isInternalOrServiceNode(node) { * * @param {!Window} win The window in which to register the custom element. * @param {(typeof ./base-element.BaseElement)=} opt_implementationClass For testing only. - * @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement element, ?(typeof BaseElement))=} opt_elementConnectedCallback + * @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement, ?(typeof BaseElement))=} opt_elementConnectedCallback * @return {!Object} Prototype of element. * @visibleForTesting */ diff --git a/src/preact/component/3p-frame.js b/src/preact/component/3p-frame.js index 4c8fb833b323..253695e86376 100644 --- a/src/preact/component/3p-frame.js +++ b/src/preact/component/3p-frame.js @@ -32,7 +32,7 @@ import {parseUrlDeprecated} from '../../url'; import {sequentialIdGenerator} from '../../utils/id-generator'; import {useLayoutEffect, useMemo, useRef, useState} from '../../../src/preact'; -/** @type {!Object} 3p frames for that type. */ +/** @type {!Object} 3p frames for that type. */ export const countGenerators = {}; /** @enum {string} */ diff --git a/src/preact/component/component.type.js b/src/preact/component/component.type.js index 78f3ca722278..df5d67339098 100644 --- a/src/preact/component/component.type.js +++ b/src/preact/component/component.type.js @@ -63,7 +63,7 @@ var IframeEmbedDef = {}; * allowFullScreen: (boolean|undefined), * allowTransparency: (boolean|undefined), * loading: (string), - * manageMessageHandler: (function({current: HTMLIFrameElement}, function):function|undefined), + * manageMessageHandler: (function({current: HTMLIFrameElement}, function():void):function():void|undefined), * name: (string|undefined), * onReadyState: (function(string)|undefined), * ready: (boolean|undefined), diff --git a/src/preact/slot.js b/src/preact/slot.js index af01b98f452c..bfda8e500573 100644 --- a/src/preact/slot.js +++ b/src/preact/slot.js @@ -142,7 +142,7 @@ export function useSlotContext(ref, opt_props) { /** * @param {!Element} slot - * @param {function(!AmpElement|!Array)} action + * @param {function(!AmpElement):void|function(!Array):void} action */ function execute(slot, action) { const assignedElements = slot.assignedElements From 701c10b0a14a8a0cdc0146e798e1c6ccfb82328d Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Fri, 30 Apr 2021 09:23:09 -0400 Subject: [PATCH 2/4] self nits --- build-system/tasks/check-types.js | 8 +++----- extensions/amp-bind/0.1/bind-impl.js | 9 +++------ 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/build-system/tasks/check-types.js b/build-system/tasks/check-types.js index ba2fec0ca788..72685ab8ea5e 100644 --- a/build-system/tasks/check-types.js +++ b/build-system/tasks/check-types.js @@ -262,10 +262,8 @@ async function typeCheck(targetName) { * of closure errors. * * This is a temporary measure while we restore 100% typechecking. - * - * @param {Array} entryPoints */ -async function ensureMinTypeQuality(entryPoints) { +async function ensureMinTypeQuality() { const srcGlobs = ['src/**/*.js', 'extensions/**/*.js']; const errorsToCheckFor = [ 'JSC_BAD_JSDOC_ANNOTATION', @@ -274,7 +272,7 @@ async function ensureMinTypeQuality(entryPoints) { ]; let msg = ''; - await closureCompile(entryPoints, './dist', `check-types.js`, { + await closureCompile('src/amp.js', './dist', `check-types.js`, { include3pDirectories: false, includePolyfills: false, typeCheckOnly: true, @@ -317,7 +315,7 @@ async function checkTypes() { log(`Checking types for targets: ${targets.map(cyan).join(', ')}`); displayLifecycleDebugging(); - await ensureMinTypeQuality(['src/amp.js']); + await ensureMinTypeQuality(); await Promise.all(targets.map(typeCheck)); exitCtrlcHandler(handlerProcess); } diff --git a/extensions/amp-bind/0.1/bind-impl.js b/extensions/amp-bind/0.1/bind-impl.js index 81a8ec8d9fe0..ed65e8494eaf 100644 --- a/extensions/amp-bind/0.1/bind-impl.js +++ b/extensions/amp-bind/0.1/bind-impl.js @@ -99,14 +99,11 @@ let BoundElementDef; /** * The options bag for binding application. - * - skipAmpState: If true, skips elements. - * - constrain: If provided, restricts application to children of the provided elements. - * - evaluateOnly: If provided, caches the evaluated result on each bound element and skips the actual DOM updates. * * @typedef {Record} ApplyOptionsDef - * @property {boolean=} skipAmpState - If true, skips elements. - * @property {Array=} constrain - * @property {boolean=} evaluateOnly + * @property {boolean=} skipAmpState If true, skips elements. + * @property {Array=} 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. */ /** From 55960f20e68bba1ea8f90db2d1b900f92ede97ea Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Mon, 3 May 2021 17:00:43 -0400 Subject: [PATCH 3/4] run prettier --- .../amp-story-dev-tools/0.1/amp-story-dev-tools-tab-preview.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 49b86e475379..f000b2438370 100644 --- 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 @@ -236,7 +236,7 @@ const MAX_DEVICE_SPACES = 4; * 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. From 6eb2b87fc585e8b4653d5af57dd664585058831b Mon Sep 17 00:00:00 2001 From: Jake Fried Date: Wed, 5 May 2021 09:10:11 -0400 Subject: [PATCH 4/4] fix a new parse error, make low-bar target --- ads/vendors/feedad.js | 2 +- build-system/tasks/check-types.js | 71 ++++++++++++------------ extensions/amp-video/1.0/video-iframe.js | 2 +- 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/ads/vendors/feedad.js b/ads/vendors/feedad.js index 1c54b64ba7ae..360263661584 100644 --- 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/tasks/check-types.js b/build-system/tasks/check-types.js index 72685ab8ea5e..33894a32d107 100644 --- a/build-system/tasks/check-types.js +++ b/build-system/tasks/check-types.js @@ -160,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': { @@ -247,52 +274,23 @@ 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)); } -/** - * Typechecks all JS files within src and extensions for a specific set - * of closure errors. - * - * This is a temporary measure while we restore 100% typechecking. - */ -async function ensureMinTypeQuality() { - const srcGlobs = ['src/**/*.js', 'extensions/**/*.js']; - const errorsToCheckFor = [ - 'JSC_BAD_JSDOC_ANNOTATION', - 'JSC_INVALID_PARAM', - 'JSC_TYPE_PARSE_ERROR', - ]; - - let msg = ''; - await closureCompile('src/amp.js', './dist', `check-types.js`, { - include3pDirectories: false, - includePolyfills: false, - typeCheckOnly: true, - extraGlobs: globby.sync(srcGlobs), - warningLevel: 'VERBOSE', - logger: (str) => (msg = str), - }).catch(() => {}); - - const targetErrors = msg - .split('\n') - .filter((s) => errorsToCheckFor.some((error) => s.includes(error))) - .join('\n') - .trim(); - - if (targetErrors.length) { - logClosureCompilerError(targetErrors); - throw new Error('Found parse errors'); - } -} - /** * Runs closure compiler's type checker against all AMP code. * @return {!Promise} @@ -315,7 +313,6 @@ async function checkTypes() { log(`Checking types for targets: ${targets.map(cyan).join(', ')}`); displayLifecycleDebugging(); - await ensureMinTypeQuality(); await Promise.all(targets.map(typeCheck)); exitCtrlcHandler(handlerProcess); } diff --git a/extensions/amp-video/1.0/video-iframe.js b/extensions/amp-video/1.0/video-iframe.js index 73e9f1fb6bb2..5be465ae4e57 100644 --- a/extensions/amp-video/1.0/video-iframe.js +++ b/extensions/amp-video/1.0/video-iframe.js @@ -193,7 +193,7 @@ export {VideoIframeInternal}; * VideoWrapper using an