fix(docs): fixup and lint optionals and return values in api.md (#1376)

Currently in our API `?` means null, but sometimes it means optional. Linting optional/nulls with this patch is required for #1166 to land nicely.

Previously, return types were not being linted in `api.md`. This is fixed, along with many broken return types.

This patch considers `?` to mean nullable, and has some heuristics to determine optionality. I believe this to be the minimal patch needed to unblock #1166. After it lands, we can consider changing the api docs to hopefully remove some heuristics and strangeness.

src/accessibility.ts

@@ -66,7 +66,7 @@ export class Accessibility {
 
   async snapshot(options: {
       interestingOnly?: boolean;
-      root?: dom.ElementHandle | null;
+      root?: dom.ElementHandle;
     } = {}): Promise<SerializedAXNode | null> {
     const {
       interestingOnly = true,

src/dom.ts

@@ -383,7 +383,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     return this._page._delegate.getBoundingBox(this);
   }
 
-  async screenshot(options?: types.ElementScreenshotOptions): Promise<string | platform.BufferType> {
+  async screenshot(options?: types.ElementScreenshotOptions): Promise<platform.BufferType> {
     return this._page._screenshotter.screenshotElement(this, options);
   }
 

src/server/browserServer.ts

@@ -20,20 +20,21 @@ import * as platform from '../platform';
 export class BrowserServer extends platform.EventEmitter {
   private _process: ChildProcess;
   private _gracefullyClose: () => Promise<void>;
-  private _browserWSEndpoint: string | null = null;
+  private _browserWSEndpoint: string = '';
 
-  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>, wsEndpoint: string | null) {
+  constructor(process: ChildProcess, gracefullyClose: () => Promise<void>, wsEndpoint: string|null) {
     super();
     this._process = process;
     this._gracefullyClose = gracefullyClose;
-    this._browserWSEndpoint = wsEndpoint;
+    if (wsEndpoint)
+      this._browserWSEndpoint = wsEndpoint;
   }
 
   process(): ChildProcess {
     return this._process;
   }
 
-  wsEndpoint(): string | null {
+  wsEndpoint(): string {
     return this._browserWSEndpoint;
   }
 

utils/doclint/check_public_api/JSBuilder.js

@@ -184,8 +184,8 @@ function checkSources(sources, externalDependencies) {
   /**
    * @param {!ts.Type} type
    */
-  function isNotNullish(type) {
-    return !((type.flags & ts.TypeFlags.Undefined) || (type.flags & ts.TypeFlags.Null));
+  function isNotUndefined(type) {
+     return !(type.flags & ts.TypeFlags.Undefined);
   }
 
   /**
@@ -234,7 +234,7 @@ function checkSources(sources, externalDependencies) {
     }
     if (type.isUnion() && (typeName.includes('|') || type.types.every(type => type.isStringLiteral() || type.intrinsicName === 'number'))) {
       const types = type.types
-        .filter(isNotNullish)
+        .filter(isNotUndefined)
         .map(type => serializeType(type, circular));
       const name = types.map(type => type.name).join('|');
       const properties = [].concat(...types.map(type => type.properties));
@@ -298,7 +298,7 @@ function checkSources(sources, externalDependencies) {
     if (signatures.length)
       return signatures[0];
     if (type.isUnion()) {
-      const innerTypes = type.types.filter(isNotNullish);
+      const innerTypes = type.types.filter(isNotUndefined);
       if (innerTypes.length === 1)
         return signatureForType(innerTypes[0]);
     }

utils/doclint/check_public_api/MDBuilder.js

@@ -47,8 +47,9 @@ class MDOutline {
 
       /**
        * @param {HTMLLIElement} element
+       * @param {boolean} defaultRequired
        */
-      function parseProperty(element) {
+      function parseProperty(element, defaultRequired) {
         const clone = element.cloneNode(true);
         const ul = clone.querySelector(':scope > ul');
         const str = parseComment(extractSiblingsIntoFragment(clone.firstChild, ul));
@@ -64,8 +65,16 @@ class MDOutline {
             const text = childElement.textContent;
             if (text.startsWith(`"`) || text.startsWith(`'`))
               continue;
-            const property = parseProperty(childElement);
-            property.required = property.comment.includes('***required***');
+            const property = parseProperty(childElement, true);
+            property.required = defaultRequired;
+            if (property.comment.toLowerCase().includes('defaults to '))
+              property.required = false;
+            if (property.comment.toLowerCase().includes('if applicable.'))
+              property.required = false;
+            if (property.comment.toLowerCase().includes('if available.'))
+              property.required = false;
+            if (property.comment.includes('**required**'))
+              property.required = true;
             properties.push(property);
           }
         }
@@ -151,13 +160,13 @@ class MDOutline {
         const ul = content.querySelector('ul');
         for (const element of content.querySelectorAll('h4 + ul > li')) {
           if (element.matches('li') && element.textContent.trim().startsWith('<')) {
-            returnType = parseProperty(element);
+            returnType = parseProperty(element, false);
           } else if (element.matches('li') && element.firstChild.matches && element.firstChild.matches('code')) {
-            const property = parseProperty(element);
+            const property = parseProperty(element, false);
             property.required = !optionalparams.has(property.name) && !property.name.startsWith('...');
             args.push(property);
           } else if (element.matches('li') && element.firstChild.nodeType === Element.TEXT_NODE && element.firstChild.textContent.toLowerCase().startsWith('return')) {
-            returnType = parseProperty(element);
+            returnType = parseProperty(element, true);
             const expectedText = 'returns: ';
             let actualText = element.firstChild.textContent;
             let angleIndex = actualText.indexOf('<');

utils/doclint/check_public_api/index.js

@@ -130,7 +130,7 @@ function compareDocumentations(actual, expected) {
           errors.push(`Method ${className}.${methodName} has unneeded description of return type`);
         else
           errors.push(`Method ${className}.${methodName} is missing return type description`);
-      } else if (actualMethod.hasReturn) {
+      } else if (actualMethod.type) {
         checkType(`Method ${className}.${methodName} has the wrong return type: `, actualMethod.type, expectedMethod.type);
       }
       const actualArgs = Array.from(actualMethod.args.keys());
@@ -143,13 +143,6 @@ function compareDocumentations(actual, expected) {
         for (const arg of argsDiff.extra)
           text.push(`- Non-existing argument found: ${arg}`);
         errors.push(text.join('\n'));
-      } else {
-        for (const name of actualMethod.args.keys()) {
-          const actual = actualMethod.args.get(name);
-          const expected = expectedMethod.args.get(name);
-          if (actual.required !== expected.required)
-            errors.push(`${className}.${methodName}(): ${name} should be ${expected.required ? 'required' : 'optional'}`);
-        }
       }
 
       for (const arg of argsDiff.equal)
@@ -182,7 +175,9 @@ function compareDocumentations(actual, expected) {
    * @param {!Documentation.Member} expected
    */
   function checkProperty(source, actual, expected) {
-    checkType(source + ' ' + actual.name, actual.type, expected.type);
+    if (actual.required !== expected.required)
+      errors.push(`${source}: ${actual.name} should be ${expected.required ? 'required' : 'optional'}`);
+    checkType(source + '.' + actual.name, actual.type, expected.type);
   }
 
   /**
@@ -196,23 +191,41 @@ function compareDocumentations(actual, expected) {
       return;
     if (expected.name === 'T' || expected.name.includes('[T]'))
       return;
-    // We don't have nullchecks on for TypeScript
-    const actualName = actual.name.replace(/[\? ]/g, '').replace(/ElementHandle\<Node\>/g, 'ElementHandle');
-    // TypeScript likes to add some spaces
-    const expectedName = expected.name.replace(/\ /g, '').replace(/ElementHandle\<Node\>/g, 'ElementHandle');
+    /** @type {Parameters<typeof String.prototype.replace>[]} */
+    const mdReplacers = [
+      [/\ /g, ''],
+      // We shortcut ? to null|
+      [/\?/g, 'null|'],
+    ];
+    const tsReplacers = [
+      [/\ /g, ''],
+      [/ElementHandle\<Element\>/g, 'ElementHandle'],
+      [/ElementHandle\<Node\>/g, 'ElementHandle'],
+      [/ElementHandle\<T\>/g, 'ElementHandle'],
+      [/Handle\<R\>/g, 'JSHandle'],
+      [/JSHandle\<Object\>/g, 'JSHandle'],
+      [/object/g, 'Object'],
+      [/Promise\<T\>/, 'Promise<Object>']
+    ]
+    let actualName = actual.name;
+    for (const replacer of mdReplacers)
+      actualName = actualName.replace(...replacer);
+    let expectedName = expected.name;
+    for (const replacer of tsReplacers)
+      expectedName = expectedName.replace(...replacer);
     if (expectedName !== actualName)
       errors.push(`${source} ${actualName} != ${expectedName}`);
     if (actual.name === 'boolean' || actual.name === 'string')
       return;
-    const actualPropertiesMap = new Map(actual.properties.map(property => [property.name, property.type]));
-    const expectedPropertiesMap = new Map(expected.properties.map(property => [property.name, property.type]));
+    const actualPropertiesMap = new Map(actual.properties.map(property => [property.name, property]));
+    const expectedPropertiesMap = new Map(expected.properties.map(property => [property.name, property]));
     const propertiesDiff = diff(Array.from(actualPropertiesMap.keys()).sort(), Array.from(expectedPropertiesMap.keys()).sort());
     for (const propertyName of propertiesDiff.extra)
       errors.push(`${source} has unexpected property '${propertyName}'`);
     for (const propertyName of propertiesDiff.missing)
       errors.push(`${source} is missing property ${propertyName}`);
     for (const propertyName of propertiesDiff.equal)
-      checkType(source + '.' + propertyName, actualPropertiesMap.get(propertyName), expectedPropertiesMap.get(propertyName));
+      checkProperty(source, actualPropertiesMap.get(propertyName), expectedPropertiesMap.get(propertyName));
   }
 
   return errors;

utils/doclint/check_public_api/test/check-nullish/api.ts

@@ -0,0 +1,19 @@
+class Foo {
+  bar(options: {x: number, y: number, maybe?: number, nullable: string|null, object?: {one: number, two?: number}}) {
+
+  }
+
+  async goBack() : Promise<Response | null> {
+    return null;
+  }
+
+  response(): Response | null {
+    return null;
+  }
+
+  baz(): {abc: number, def?: number, ghi: string} | null {
+    return null;
+  }
+}
+
+export {Foo};

utils/doclint/check_public_api/test/check-nullish/doc.md

@@ -0,0 +1,32 @@
+### class: Foo
+
+#### foo.bar(options)
+- `options` <[Object]>
+  - `x` <[number]>  **required**
+  - `y` <[number]>  **required**
+  - `nullable` <?[string]>  **required**
+  - `maybe` <[number]>
+  - `object` <[Object]>
+    - `one` <[number]>
+    - `two` <[number]> defaults to `2`.
+
+#### foo.baz()
+- returns: <?[Object]>
+  - `abc` <[number]>
+  - `def` <[number]> if applicable.
+  - `ghi` <[string]>
+
+
+#### foo.goBack()
+- returns: <[Promise]<?[Response]>> Promise which resolves to the main resource response. In case of multiple redirects, the navigation will resolve with the response of the last redirect. If
+can not go back, resolves to `null`.
+
+#### foo.response()
+- returns: <?[Response]> A matching [Response] object, or `null` if the response has not been received yet.
+
+
+[string]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type "String"
+[Object]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Object_type "Object"
+[number]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#number_type "number"
+[Promise]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Promise_type "Promise"
+[Response]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Response_type "Response"

utils/doclint/check_public_api/test/check-returns/api.ts

@@ -10,9 +10,8 @@ class Foo {
     e();
   }
 
-  www() {
-    if (1 === 1)
-      return 'df';
+  www() : string {
+    return 'df';
   }
 
   async asyncFunction() {

utils/doclint/check_public_api/test/test.js

@@ -51,6 +51,7 @@ describe('checkPublicAPI', function() {
   it('check-duplicates', testLint);
   it('check-sorting', testLint);
   it('check-returns', testLint);
+  it('check-nullish', testLint);
   it('js-builder-common', testJSBuilder);
   it('js-builder-inheritance', testJSBuilder);
   it('md-builder-common', testMDBuilder);