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

JoelEinbinder · 2020-03-13T12:27:42Z

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.

pavelfeldman · 2020-03-13T15:07:04Z

utils/doclint/cli.js

-      firefoxVersion: browserVersions.firefox,
-    })));
-    messages.push(...await preprocessor.ensureReleasedAPILinks([readme], VERSION));
+    // const preprocessor = require('./preprocessor');


What is this about?

draft, its not done.

dgozman

Can we spell out our policy? Something like the following or not?

Optional arguments are denoted as [, arg] in the arg list.
All incoming object properties are optional by default.
All incoming object properties which are required are marked as required.
Outgoing object properties can not be optional (only nullable).
"?" means nullable, both in incoming and outgoing.

dgozman · 2020-03-13T17:13:22Z

docs/api.md

@@ -205,7 +205,7 @@ Indicates that the browser is connected.
  - `geolocation` <[Object]>
    - `latitude` <[number]> Latitude between -90 and 90.


Required here?

dgozman · 2020-03-13T17:14:10Z

docs/api.md

@@ -3718,7 +3718,7 @@ const { chromium } = require('playwright');  // Or 'firefox' or 'webkit'.

 #### browserType.connect(options)
 - `options` <[Object]>
-  - `wsEndpoint` <[string]> A browser websocket endpoint to connect to.
+  - `wsEndpoint` <[string]> A browser websocket endpoint to connect to. **required**
  - `slowMo` <?[number]> Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on. Defaults to 0.


Remove '?' here?

dgozman · 2020-03-17T20:47:29Z

docs/api.md

@@ -1750,7 +1749,7 @@ const [response] = await Promise.all([
 Shortcut for [page.mainFrame().waitForNavigation(options)](#framewaitfornavigationoptions).

 #### page.waitForRequest(urlOrPredicate[, options])
- `urlOrPredicate` <?[string]|[RegExp]|[Function]> Optional. Request URL string, regex or predicate receiving [Request] object.
+- `urlOrPredicate` <[string]|[RegExp]|[Function]> Optional. Request URL string, regex or predicate receiving [Request] object.


Reading the implementation, urlOrPredicate is not optional. Let's update the description.

dgozman · 2020-03-17T20:47:34Z

docs/api.md

@@ -1766,7 +1765,7 @@ await page.waitForRequest(request => request.url().searchParams.get('foo') === '
 ```

 #### page.waitForResponse(urlOrPredicate[, options])
- `urlOrPredicate` <?[string]|[RegExp]|[Function]> Optional. Request URL string, regex or predicate receiving [Response] object.
+- `urlOrPredicate` <[string]|[RegExp]|[Function]> Optional. Request URL string, regex or predicate receiving [Response] object.


dgozman · 2020-03-17T20:48:43Z

docs/api.md

@@ -2931,9 +2930,9 @@ function, it **will not be called**.

 #### consoleMessage.location()
 - returns: <[Object]>


We return an object with optional fields? Let's make them nullable instead.

These are helpful when JSON stringifying or console logging these objects in node. I'd rather not change implementation in this patch.

dgozman · 2020-03-17T20:49:56Z

docs/api.md

-  - `invalid` <[string]> Whether and in what way this node's value is invalid.
-  - `orientation` <[string]> Whether the node is oriented horizontally or vertically.
-  - `children` <[Array]<[Object]>> Child [AXNode]s of this node, if any.
+  - `value` <[string]|[number]> The current value of the node, if applicable.


Same here - make it nullable?

dgozman · 2020-03-17T20:50:21Z

docs/api.md

@@ -3905,7 +3904,7 @@ const v8toIstanbul = require('v8-to-istanbul');
 #### chromiumCoverage.stopCSSCoverage()
 - returns: <[Promise]<[Array]<[Object]>>> Promise that resolves to the array of coverage reports for all stylesheets
  - `url` <[string]> StyleSheet URL
-  - `text` <[string]> StyleSheet content
+  - `text` <[string]> StyleSheet content, if available.


dgozman · 2020-03-17T20:50:26Z

docs/api.md

@@ -3915,8 +3914,13 @@ const v8toIstanbul = require('v8-to-istanbul');
 #### chromiumCoverage.stopJSCoverage()
 - returns: <[Promise]<[Array]<[Object]>>> Promise that resolves to the array of coverage reports for all scripts
  - `url` <[string]> Script URL
-  - `source` <[string]> Script content
+  - `source` <[string]> Script content, if applicable.


pavelfeldman reviewed Mar 13, 2020

View reviewed changes

dgozman reviewed Mar 13, 2020

View reviewed changes

JoelEinbinder force-pushed the fixup_api_optionals branch 3 times, most recently from a340ba7 to 9622d70 Compare March 17, 2020 20:31

JoelEinbinder changed the title ~~fix optionals in api.md~~ fix(docs): fixup and lint optionals and return values in api.md Mar 17, 2020

JoelEinbinder marked this pull request as ready for review March 17, 2020 20:41

dgozman approved these changes Mar 17, 2020

View reviewed changes

JoelEinbinder force-pushed the fixup_api_optionals branch from 9622d70 to 0dbe973 Compare March 17, 2020 23:31

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

482121b

JoelEinbinder force-pushed the fixup_api_optionals branch from 0dbe973 to 482121b Compare March 18, 2020 19:15

urlOrPredicate isn't optional

9bf9d34

JoelEinbinder merged commit b0749e3 into microsoft:master Mar 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JoelEinbinder commented Mar 13, 2020 •

edited

Loading

pavelfeldman Mar 13, 2020

JoelEinbinder Mar 14, 2020

dgozman left a comment

dgozman Mar 13, 2020

dgozman Mar 13, 2020

dgozman Mar 17, 2020

JoelEinbinder Mar 18, 2020

dgozman Mar 17, 2020

JoelEinbinder Mar 18, 2020

dgozman Mar 17, 2020

JoelEinbinder Mar 18, 2020 •

edited

Loading

dgozman Mar 17, 2020

dgozman Mar 17, 2020

dgozman Mar 17, 2020

		@@ -205,7 +205,7 @@ Indicates that the browser is connected.
		- `geolocation` <[Object]>
		- `latitude` <[number]> Latitude between -90 and 90.

		@@ -2931,9 +2930,9 @@ function, it will not be called.

		#### consoleMessage.location()
		- returns: <[Object]>

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

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

Conversation

JoelEinbinder commented Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgozman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelEinbinder Mar 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelEinbinder commented Mar 13, 2020 •

edited

Loading

JoelEinbinder Mar 18, 2020 •

edited

Loading