Skip to content

Commit 1dfbaff

Browse files
authoredMay 5, 2021
Types: fix all type parse errors and ensure no new ones crop up (#34105)
* Types: fix all type parse errors * self nits * run prettier * fix a new parse error, make low-bar target
1 parent 8d60de5 commit 1dfbaff

File tree

26 files changed

+133
-70
lines changed

26 files changed

+133
-70
lines changed
 

‎ads/vendors/feedad.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ import {setStyle} from '../../src/style';
5757
*/
5858

5959
/**
60-
* @param {!FeedAdGlobal & Window} global
60+
* @param {!FeedAdGlobal} global
6161
* @param {!FeedAdData} data
6262
*/
6363
export function feedad(global, data) {

‎build-system/compile/closure-compile.js

+11-4
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ function logClosureCompilerError(message) {
5252
*/
5353
function handleClosureCompilerError(err, outputFilename, options) {
5454
if (options.typeCheckOnly) {
55-
log(`${red('ERROR:')} Type checking failed`);
55+
if (!options.logger) {
56+
log(`${red('ERROR:')} Type checking failed`);
57+
}
5658
return err;
5759
}
5860
log(`${red('ERROR:')} Could not minify ${cyan(outputFilename)}`);
@@ -69,10 +71,14 @@ function handleClosureCompilerError(err, outputFilename, options) {
6971
* on Windows exceeds the command line size limit. The stream mode is 'IN'
7072
* because output files and sourcemaps are written directly to disk.
7173
* @param {Array<string>} flags
74+
* @param {!Object} options
7275
* @return {!Object}
7376
*/
74-
function initializeClosure(flags) {
75-
const pluginOptions = {streamMode: 'IN', logger: logClosureCompilerError};
77+
function initializeClosure(flags, options) {
78+
const pluginOptions = {
79+
streamMode: 'IN',
80+
logger: options.logger || logClosureCompilerError,
81+
};
7682
return compiler.gulp()(flags, pluginOptions);
7783
}
7884

@@ -88,7 +94,7 @@ function runClosure(outputFilename, options, flags, srcFiles) {
8894
return new Promise((resolve, reject) => {
8995
vinylFs
9096
.src(srcFiles, {base: getBabelOutputDir()})
91-
.pipe(initializeClosure(flags))
97+
.pipe(initializeClosure(flags, options))
9298
.on('error', (err) => {
9399
const reason = handleClosureCompilerError(err, outputFilename, options);
94100
reason ? reject(reason) : resolve();
@@ -99,5 +105,6 @@ function runClosure(outputFilename, options, flags, srcFiles) {
99105
}
100106

101107
module.exports = {
108+
logClosureCompilerError,
102109
runClosure,
103110
};

‎build-system/tasks/check-types.js

+35
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ const {compileCss} = require('./css');
2828
const {compileJison} = require('./compile-jison');
2929
const {cyan, green, yellow, red} = require('kleur/colors');
3030
const {extensions, maybeInitializeExtensions} = require('./extension-helpers');
31+
const {logClosureCompilerError} = require('../compile/closure-compile');
3132
const {log} = require('../common/logging');
3233
const {typecheckNewServer} = require('../server/typescript-compile');
3334

@@ -159,6 +160,33 @@ const TYPE_CHECK_TARGETS = {
159160
externGlobs: [CORE_EXTERNS_GLOB, 'build-system/externs/*.extern.js'],
160161
},
161162

163+
/*
164+
* Ensures that all files in src and extensions pass the specified set of errors.
165+
*/
166+
'low-bar': {
167+
entryPoints: ['src/amp.js'],
168+
extraGlobs: ['{src,extensions}/**/*.js'],
169+
onError(msg) {
170+
const lowBarErrors = [
171+
'JSC_BAD_JSDOC_ANNOTATION',
172+
'JSC_INVALID_PARAM',
173+
'JSC_TYPE_PARSE_ERROR',
174+
];
175+
const lowBarRegex = new RegExp(lowBarErrors.join('|'));
176+
177+
const targetErrors = msg
178+
.split('\n')
179+
.filter((s) => lowBarRegex.test(s))
180+
.join('\n')
181+
.trim();
182+
183+
if (targetErrors.length) {
184+
logClosureCompilerError(targetErrors);
185+
throw new Error(`Type-checking failed for target ${cyan('low-bar')}`);
186+
}
187+
},
188+
},
189+
162190
// TODO(#33631): Targets below this point are not expected to pass.
163191
// They can possibly be removed?
164192
'src': {
@@ -246,12 +274,19 @@ async function typeCheck(targetName) {
246274
return;
247275
}
248276

277+
let errorMsg;
249278
await closureCompile(entryPoints, './dist', `${targetName}-check-types.js`, {
250279
noAddDeps,
251280
include3pDirectories: !noAddDeps,
252281
includePolyfills: !noAddDeps,
253282
typeCheckOnly: true,
283+
logger: (m) => (errorMsg = m),
254284
...opts,
285+
}).catch((error) => {
286+
if (!target.onError) {
287+
throw error;
288+
}
289+
target.onError(errorMsg);
255290
});
256291
log(green('SUCCESS:'), 'Type-checking passed for target', cyan(targetName));
257292
}

‎extensions/amp-a4a/0.1/amp-ad-utils.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export function getAmpAdMetadata(creative) {
167167
/**
168168
* Merges any elements from customElementExtensions array into extensions array if
169169
* the element is not present.
170-
* @param {!Array<{custom-element: string, 'src': string}} extensions
170+
* @param {!Array<{custom-element: string, 'src': string}>} extensions
171171
* @param {!Array<string>} customElementExtensions
172172
*/
173173
export function mergeExtensionsMetadata(extensions, customElementExtensions) {

‎extensions/amp-accordion/1.0/component.type.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ AccordionDef.AccordionHeaderProps;
5757
/**
5858
* @typedef {{
5959
* as: (string|PreactDef.FunctionalComponent|undefined),
60-
* role: (string|undefined)
60+
* role: (string|undefined),
6161
* className: (string|undefined),
6262
* id: (string|undefined),
6363
* children: (?PreactDef.Renderable|undefined),

‎extensions/amp-addthis/0.1/amp-addthis.js

+9-4
Original file line numberDiff line numberDiff line change
@@ -403,13 +403,18 @@ class AmpAddThis extends AMP.BaseElement {
403403
}, {});
404404
}
405405

406+
/**
407+
* @typedef {{
408+
* ampdoc: !../../../src/service/ampdoc-impl.AmpDoc,
409+
* loc: *,
410+
* pubId: *,
411+
* }} SetupListenersInput
412+
*/
413+
406414
/**
407415
* Sets up listeners.
408416
*
409-
* @param {!Object} input
410-
* @param {!../../../src/service/ampdoc-impl.AmpDoc} [input.ampdoc]
411-
* @param {*} [input.loc]
412-
* @param {*} [input.pubId]
417+
* @param {!SetupListenersInput} input
413418
* @memberof AmpAddThis
414419
*/
415420
setupListeners_(input) {

‎extensions/amp-addthis/0.1/config-manager.js

+13-8
Original file line numberDiff line numberDiff line change
@@ -111,14 +111,19 @@ export class ConfigManager {
111111
}
112112

113113
/**
114-
* @param {!Object} input
115-
* @param {*} [input.iframe]
116-
* @param {string} [input.widgetId]
117-
* @param {string} [input.pubId]
118-
* @param {!Object} [input.shareConfig]
119-
* @param {!Object} [input.atConfig]
120-
* @param {string} [input.productCode]
121-
* @param {string} [input.containerClassName]
114+
* @typedef {{
115+
* iframe: *,
116+
* widgetId: string,
117+
* pubId: string,
118+
* shareConfig: !Object,
119+
* atConfig: !Object,
120+
* productCode: string,
121+
* containerClassName: string,
122+
* }} SendConfigurationInput
123+
*/
124+
125+
/**
126+
* @param {!SendConfigurationInput} input
122127
* @private
123128
*/
124129
sendConfiguration_(input) {

‎extensions/amp-bind/0.1/bind-impl.js

+10-6
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,15 @@ let BoundPropertyDef;
9797
*/
9898
let BoundElementDef;
9999

100+
/**
101+
* The options bag for binding application.
102+
*
103+
* @typedef {Record} ApplyOptionsDef
104+
* @property {boolean=} skipAmpState If true, skips <amp-state> elements.
105+
* @property {Array<!Element>=} constrain If provided, restricts application to children of the provided elements.
106+
* @property {boolean=} evaluateOnly If provided, caches the evaluated result on each bound element and skips the actual DOM updates.
107+
*/
108+
100109
/**
101110
* A map of tag names to arrays of attributes that do not have non-bind
102111
* counterparts. For instance, amp-carousel allows a `[slide]` attribute,
@@ -1188,12 +1197,7 @@ export class Bind {
11881197
* Applies expression results to elements in the document.
11891198
*
11901199
* @param {Object<string, BindExpressionResultDef>} results
1191-
* @param {!Object} opts
1192-
* @param {boolean=} opts.skipAmpState If true, skips <amp-state> elements.
1193-
* @param {Array<!Element>=} opts.constrain If provided, restricts application
1194-
* to children of the provided elements.
1195-
* @param {boolean=} opts.evaluateOnly If provided, caches the evaluated
1196-
* result on each bound element and skips the actual DOM updates.
1200+
* @param {!ApplyOptionsDef} opts
11971201
* @return {!Promise}
11981202
* @private
11991203
*/

‎extensions/amp-brightcove/0.1/amp-brightcove.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ class AmpBrightcove extends AMP.BaseElement {
142142
}
143143

144144
/**
145-
* @return {Promise[]}
145+
* @return {Promise}
146146
*/
147147
getConsents_() {
148148
const consentPolicy = super.getConsentPolicy();

‎extensions/amp-lightbox-gallery/1.0/component.type.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ LightboxGalleryDef.Props;
3232
* as: (string|undefined),
3333
* children: (!PreactDef.Renderable),
3434
* enableActivation: (boolean|undefined),
35-
* onClick: (function(Event)|undefined)
35+
* onClick: (function(Event)|undefined),
3636
* render: (function():PreactDef.Renderable),
3737
* }}
3838
*/
@@ -42,7 +42,7 @@ LightboxGalleryDef.WithLightboxProps;
4242
* @typedef {{
4343
* deregister: (function(string):undefined),
4444
* register: (function(string, Element):undefined),
45-
* open: (function:undefined),
45+
* open: (function():undefined),
4646
* }}
4747
*/
4848
LightboxGalleryDef.ContextProps;

‎extensions/amp-lightbox/1.0/component.type.js

+7-7
Original file line numberDiff line numberDiff line change
@@ -23,27 +23,27 @@ var LightboxDef = {};
2323
* id: (string),
2424
* animation: (string|undefined),
2525
* children: (?PreactDef.Renderable|undefined),
26-
* closeButtonAs: (function:PreactDef.Renderable|undefined),
26+
* closeButtonAs: (function():PreactDef.Renderable|undefined),
2727
* scrollable: (boolean),
2828
* initialOpen: (boolean),
29-
* onBeforeOpen: (function|undefined),
30-
* onAfterOpen: (function|undefined),
31-
* onAfterClose: (function|undefined),
29+
* onBeforeOpen: (function():void|undefined),
30+
* onAfterOpen: (function():void|undefined),
31+
* onAfterClose: (function():void|undefined),
3232
* }}
3333
*/
3434
LightboxDef.Props;
3535

3636
/**
3737
* @typedef {{
3838
* aria-label: (string),
39-
* as: (function:PreactDef.Renderable|undefined),
40-
* onClick: function,
39+
* as: (function():PreactDef.Renderable|undefined),
40+
* onClick: function():void,
4141
* }}
4242
*/
4343
LightboxDef.CloseButtonProps;
4444

4545
/** @interface */
46-
Lightbox.LightboxApi = class {
46+
LightboxDef.LightboxApi = class {
4747
/** Open the lightbox */
4848
open() {}
4949

‎extensions/amp-render/1.0/component.type.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ var RenderDef = {};
2222
/**
2323
* @typedef {{
2424
* src: (!string),
25-
* getJson: (!Function)
25+
* getJson: (!Function),
2626
* render: (?RendererFunctionType|undefined),
2727
* }}
2828
*/

‎extensions/amp-selector/1.0/component.type.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ SelectorDef.OptionProps;
5656
/**
5757
* @typedef {{
5858
* disabled: (boolean|undefined),
59-
* focusRef: ({current: {active: *, focusMap: !Object}),
59+
* focusRef: ({current: {active: *, focusMap: !Object}}),
6060
* keyboardSelectMode: (string|undefined),
6161
* multiple: (boolean|undefined),
6262
* optionsRef: ({current: !Array<*>}),

‎extensions/amp-sidebar/1.0/component.type.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,9 @@ var SidebarDef = {};
2323
* @typedef {{
2424
* as: (string|PreactDef.FunctionalComponent|undefined),
2525
* side: (string|undefined),
26-
* onBeforeOpen: (function|undefined),
27-
* onAfterOpen: (function|undefined),
28-
* onAfterClose: (function|undefined),
26+
* onBeforeOpen: (function():void|undefined),
27+
* onAfterOpen: (function():void|undefined),
28+
* onAfterClose: (function():void|undefined),
2929
* backdropStyle: (?Object|undefined),
3030
* backdropClassName: (string|undefined),
3131
* children: (?PreactDef.Renderable|undefined),

‎extensions/amp-sidebar/1.0/sidebar-animations-hook.js

+5-5
Original file line numberDiff line numberDiff line change
@@ -49,12 +49,12 @@ function safelySetStyles(element, styles) {
4949
/**
5050
* @param {boolean} mounted
5151
* @param {boolean} opened
52-
* @param {{current: function|undefined}} onAfterOpen
53-
* @param {{current: function|undefined}} onAfterClose
52+
* @param {{current: function():void} | {current: void}} onAfterOpen
53+
* @param {{current: function():void} | {current: void}} onAfterClose
5454
* @param {string} side
55-
* @param {{current: Element|null}} sidebarRef
56-
* @param {{current: Element|null}} backdropRef
57-
* @param {function} setMounted
55+
* @param {{current: (Element|null)}} sidebarRef
56+
* @param {{current: (Element|null)}} backdropRef
57+
* @param {function():undefined} setMounted
5858
*/
5959
export function useSidebarAnimation(
6060
mounted,

‎extensions/amp-story-auto-ads/0.1/story-ad-page-manager.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ export class StoryAdPageManager {
6969
/** @private {!./story-ad-button-text-fitter.ButtonTextFitter} */
7070
this.buttonFitter_ = new ButtonTextFitter(this.ampdoc_);
7171

72-
/** @private {Object<string, StoryAdPage} */
72+
/** @private {Object<string, StoryAdPage>} */
7373
this.pages_ = {};
7474

7575
/** @private {!../../amp-story/1.0/amp-story-store-service.AmpStoryStoreService} **/

‎extensions/amp-story-auto-ads/0.1/story-ad-ui.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ export function getStoryAdMetadataFromElement(adElement) {
111111
/**
112112
* Returns a boolean indicating if there is sufficent metadata to render CTA.
113113
* @param {!StoryAdUIMetadata} metadata
114-
* @param {=boolean} opt_inabox
114+
* @param {boolean=} opt_inabox
115115
* @return {boolean}
116116
*/
117117
export function validateCtaMetadata(metadata, opt_inabox) {

‎extensions/amp-story-dev-tools/0.1/amp-story-dev-tools-tab-preview.js

+4-4
Original file line numberDiff line numberDiff line change
@@ -229,18 +229,18 @@ const MAX_DEVICE_SPACES = 4;
229229
/**
230230
* @typedef {{
231231
* element: !Element,
232-
* player: !Element
232+
* player: !Element,
233233
* chip: !Element,
234234
* width: number,
235235
* height: number,
236236
* deviceHeight: ?number,
237237
* deviceSpaces: number,
238-
* }}
238+
* }} DeviceInfo
239+
*
239240
* Contains the data related to the device.
240241
* Width and height refer to the story viewport, while deviceHeight is the device screen height.
241242
* The deviceSpaces refers to the MAX_DEVICE_SPACES, ensuring the devices on screen don't go over the max space set.
242243
*/
243-
export let DeviceInfo;
244244

245245
const DEFAULT_DEVICES = 'iphone11native;oneplus5t;pixel2';
246246

@@ -363,7 +363,7 @@ function simplifyDeviceName(name) {
363363
* Eg: `devices="ipad;iphone"` will find the ipad and also the first device in ALL_DEVICES
364364
* that starts with "iphone" (ignoring case and symbols).
365365
* @param {string} queryHash
366-
* @return {any[]}
366+
* @return {Array<*>}
367367
*/
368368
function parseDevices(queryHash) {
369369
const screenSizes = [];

‎extensions/amp-subscriptions/0.1/entitlement.js

+15-8
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,20 @@ export const GrantReason = {
2323
'LAA': 'LAA',
2424
};
2525

26+
/**
27+
* The constructor arg for an {@link Entitlement}
28+
*
29+
* @typedef {{
30+
* source: string,
31+
* raw: string,
32+
* service: string,
33+
* granted: boolean,
34+
* grantReason: ?GrantReason,
35+
* dataObject: ?JsonObject,
36+
* decryptedDocumentKey: ?string
37+
* }} EntitlementConstructorInputDef
38+
*/
39+
2640
/**
2741
* The single entitlement object.
2842
*/
@@ -41,14 +55,7 @@ export class Entitlement {
4155
}
4256

4357
/**
44-
* @param {Object} input
45-
* @param {string} [input.source]
46-
* @param {string} [input.raw]
47-
* @param {string} [input.service]
48-
* @param {boolean} [input.granted]
49-
* @param {?GrantReason} [input.grantReason]
50-
* @param {?JsonObject} [input.dataObject]
51-
* @param {?string} [input.decryptedDocumentKey]
58+
* @param {!EntitlementConstructorInputDef} input
5259
*/
5360
constructor(input) {
5461
const {

‎extensions/amp-video/1.0/video-iframe.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ export {VideoIframeInternal};
193193
* VideoWrapper using an <iframe> for implementation.
194194
* Usable on the AMP layer through VideoBaseElement.
195195
* @param {VideoIframeDef.Props} props
196-
* @param {{current: T|null}} ref
196+
* @param {{current: (T|null)}} ref
197197
* @return {PreactDef.Renderable}
198198
* @template T
199199
*/

‎src/amp-story-player/amp-story-player-viewport-observer.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ export class AmpStoryPlayerViewportObserver {
2727
/**
2828
* @param {!Window} win
2929
* @param {!Element} element
30-
* @param {function} viewportCb
30+
* @param {function():void} viewportCb
3131
*/
3232
constructor(win, element, viewportCb) {
3333
/** @private {!Window} */
@@ -36,10 +36,10 @@ export class AmpStoryPlayerViewportObserver {
3636
/** @private {!Element} */
3737
this.element_ = element;
3838

39-
/** @private {function} */
39+
/** @private {function():void} */
4040
this.cb_ = viewportCb;
4141

42-
/** @private {?function} */
42+
/** @private {?function():void} */
4343
this.scrollHandler_ = null;
4444

4545
this.initializeInObOrFallback_();

‎src/context/node.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -396,7 +396,7 @@ export class ContextNode {
396396
* of factory is important to reduce bundling costs for context node.
397397
*
398398
* @param {*} id
399-
* @param {fucntion(new:./subscriber.Subscriber, function(...?), !Array<!ContextProp>)} constr
399+
* @param {function(new:./subscriber.Subscriber, function(...?), !Array<!ContextProp>):void} constr
400400
* @param {!Function} func
401401
* @param {!Array<!ContextProp>} deps
402402
*/

‎src/custom-element.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ function isTemplateTagSupported() {
8989
* Creates a named custom element class.
9090
*
9191
* @param {!Window} win The window in which to register the custom element.
92-
* @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement element, ?(typeof BaseElement))} elementConnectedCallback
92+
* @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement, ?(typeof BaseElement))} elementConnectedCallback
9393
* @return {typeof AmpElement} The custom element class.
9494
*/
9595
export function createCustomElementClass(win, elementConnectedCallback) {
@@ -107,7 +107,7 @@ export function createCustomElementClass(win, elementConnectedCallback) {
107107
* Creates a base custom element class.
108108
*
109109
* @param {!Window} win The window in which to register the custom element.
110-
* @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement element, ?(typeof BaseElement))} elementConnectedCallback
110+
* @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement, ?(typeof BaseElement))} elementConnectedCallback
111111
* @return {typeof HTMLElement}
112112
*/
113113
function createBaseCustomElementClass(win, elementConnectedCallback) {
@@ -2177,7 +2177,7 @@ function isInternalOrServiceNode(node) {
21772177
*
21782178
* @param {!Window} win The window in which to register the custom element.
21792179
* @param {(typeof ./base-element.BaseElement)=} opt_implementationClass For testing only.
2180-
* @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement element, ?(typeof BaseElement))=} opt_elementConnectedCallback
2180+
* @param {function(!./service/ampdoc-impl.AmpDoc, !AmpElement, ?(typeof BaseElement))=} opt_elementConnectedCallback
21812181
* @return {!Object} Prototype of element.
21822182
* @visibleForTesting
21832183
*/

‎src/preact/component/3p-frame.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ import {parseUrlDeprecated} from '../../url';
3232
import {sequentialIdGenerator} from '../../utils/id-generator';
3333
import {useLayoutEffect, useMemo, useRef, useState} from '../../../src/preact';
3434

35-
/** @type {!Object<string,function>} 3p frames for that type. */
35+
/** @type {!Object<string,function():void>} 3p frames for that type. */
3636
export const countGenerators = {};
3737

3838
/** @enum {string} */

‎src/preact/component/component.type.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ var IframeEmbedDef = {};
6363
* allowFullScreen: (boolean|undefined),
6464
* allowTransparency: (boolean|undefined),
6565
* loading: (string),
66-
* manageMessageHandler: (function({current: HTMLIFrameElement}, function):function|undefined),
66+
* manageMessageHandler: (function({current: HTMLIFrameElement}, function():void):function():void|undefined),
6767
* name: (string|undefined),
6868
* onReadyState: (function(string)|undefined),
6969
* ready: (boolean|undefined),

‎src/preact/slot.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ export function useSlotContext(ref, opt_props) {
142142

143143
/**
144144
* @param {!Element} slot
145-
* @param {function(!AmpElement|!Array<!AmpElement>)} action
145+
* @param {function(!AmpElement):void|function(!Array<!AmpElement>):void} action
146146
*/
147147
function execute(slot, action) {
148148
const assignedElements = slot.assignedElements

0 commit comments

Comments
 (0)
Please sign in to comment.