types: better types #1166

JoelEinbinder · 2020-02-29T00:37:04Z

#6

A place for discussion

JoelEinbinder · 2020-02-29T01:33:32Z

@dgozman comments
In events:

arg0: void should be ()
arg0 should be camelcase type name
overrides of interface types
enums somehow
combine as many types as possible
parser broke on FirefoxBrowser events
page being templated is ugly
tag names are not needed
HTMLElement/SVGElement by default

JoelEinbinder · 2020-03-04T01:43:23Z

I'll try to do enums in a follow up, they are difficult.
Combining types makes it hard to guarantee no breaking changes. In a follow up, ill try to move a lot of our duplicates into aliases though, which should make things more readable.
I kept the tags because I like them a lot.

dgozman · 2020-03-04T17:16:10Z

index.d.ts

-export const chromium: import('./lib/server/chromium').Chromium;
-export const firefox: import('./lib/server/firefox').Firefox;
-export const webkit: import('./lib/server/webkit').WebKit;
-export const selectors: import('./lib/api').Selectors;


What about selectors, web, errors and devices?

Is web public right now? How is it accessed? I did the others.

dgozman · 2020-03-04T17:16:42Z

package.json

@@ -30,9 +30,10 @@
    "wcoverage": "cross-env COVERAGE=true BROWSER=webkit node test/test.js",
    "tsc": "tsc -p .",
    "clean": "rimraf lib",
-    "build": "node utils/runWebpack.js --mode='development' && tsc -p .",
+    "build": "node utils/runWebpack.js --mode='development' && tsc -p . && npm run generate-types",
    "watch": "node utils/runWebpack.js --mode='development' --watch --silent | tsc -w -p .",


Do we need something for watch?

I'm not sure. I don't think it works right now in build though. It depends on the protocol.ts file to be generated, but the protocol generator requires the project to be built. I'll move it to a separate command that runs on bots, test, and before publishing.

utils/generate_types/index.js

utils/generate_types/overrides.d.ts

utils/generate_types/parseOverrides.js

utils/generate_types/test/test.ts

utils/generate_types/index.js

JoelEinbinder · 2020-03-11T00:05:29Z

I think this is ready for another look!

dgozman

Overall looks good. Could you please post a gist with generated types?

utils/generate_types/overrides.d.ts

dgozman · 2020-03-11T04:01:18Z

utils/generate_types/overrides.d.ts

+type ElementHandleForTag<K extends keyof HTMLElementTagNameMap> = ElementHandle<HTMLElementTagNameMap[K]>;
+
+type WaitForSelectorOptionsNotHidden = PageWaitForSelectorOptions & {
+  visibility: 'visible'|'any';


This has been changed recently.

Unfortunately my tests for this didn't catch it somehow. I don't know how to make them more strict, but I fixed it.

utils/generate_types/overrides.d.ts

JoelEinbinder · 2020-03-11T22:17:01Z

https://gist.github.com/JoelEinbinder/d5086a918a7886fdfc2f9d6dafb29623

dgozman · 2020-03-12T03:43:05Z

https://gist.github.com/JoelEinbinder/d5086a918a7886fdfc2f9d6dafb29623

FirefoxBrowser overrides disconnected event.
WebKitBrowserNewPageOptions.ignoreHTTPSErrors allows null value, but we actually don't expect that, only undefined. Same with other options.
Will you follow up with declaring type FirefoxFooOptions = GenericFooOptions?
Many option fields have JSDoc not starting with capital letter. Perhaps we should fix the docs?
I am surprised with BrowserContextSetHTTPCredentialsOptions. Do we call any object parameters "Options"? Do we ever have two object parameters?
ElementHandleTripleclickOptionsOffset has optional x and y, should be required.
PageFilechooserPayload is defined 3 times.

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.

JoelEinbinder · 2020-03-20T00:43:53Z

Ready for another look!

New gist: https://gist.github.com/JoelEinbinder/4d69cd5373d00443c1b5efa72ae114bd

FirefoxBrowser overrides disconnected event.

Done

WebKitBrowserNewPageOptions.ignoreHTTPSErrors allows null value, but we actually don't expect that, only undefined. Same with other options.

Done via #1206

Will you follow up with declaring type FirefoxFooOptions = GenericFooOptions?

Yes

Many option fields have JSDoc not starting with capital letter. Perhaps we should fix the docs?

English review of the api is up next. I'll fix it then.

I am surprised with BrowserContextSetHTTPCredentialsOptions. Do we call any object parameters "Options"? Do we ever have two object parameters?

I was using 'Options' everywhere. I changed it to the argument name. This works better everywhere except for PageWaitForEventOptionsOrPredicate, but for the sake of consistency I'm not going to worry about that.

Note that this means changing the argument names could be a breaking change, because the types will change.

ElementHandleTripleclickOptionsOffset has optional x and y, should be required.

Done via #1206

PageFilechooserPayload is defined 3 times.

Done

dgozman · 2020-03-20T03:24:01Z

I am surprised with BrowserContextSetHTTPCredentialsOptions. Do we call any object parameters "Options"? Do we ever have two object parameters?

I was using 'Options' everywhere. I changed it to the argument name. This works better everywhere except for PageWaitForEventOptionsOrPredicate, but for the sake of consistency I'm not going to worry about that.

Note that this means changing the argument names could be a breaking change, because the types will change.

I don't like this restriction. Let's brainstorm and follow up.

dgozman

Let's land it!

dgozman · 2020-03-20T03:20:28Z

test/types.d.ts

@@ -48,7 +48,7 @@ interface TestSetup<STATE> {
    MAC: boolean;
    LINUX: boolean;
    WIN: boolean;
-    playwright: import('../src/server/browserType').BrowserType;
+    playwright: import('../src/server/browserType').BrowserType<import('../src/browser').Browser>;


Let's migrate this whole file to public types in a follow up.

Definitely!

dgozman · 2020-03-20T03:21:32Z

utils/generate_types/index.js

+const objectDefinitions = [];
+const handledMethods = new Set();
+/** @type {Documentation} */
+let documentation;


WDYT about slowly rewriting all our utils into typescript? Having types will definitely help.

We have types through JSDoc. I'm always against typescript, so I am biased. With the utils, is harder because the utils will depend on the library to be built and the library depends on the utils to be built, so you end up needing to require the npm playwright as a devDependency of playwright or having separate bootstrap logic. I'd rather avoid it.

JoelEinbinder · 2020-03-20T08:27:32Z

I am surprised with BrowserContextSetHTTPCredentialsOptions. Do we call any object parameters "Options"? Do we ever have two object parameters?

I was using 'Options' everywhere. I changed it to the argument name. This works better everywhere except for PageWaitForEventOptionsOrPredicate, but for the sake of consistency I'm not going to worry about that.
Note that this means changing the argument names could be a breaking change, because the types will change.

I don't like this restriction. Let's brainstorm and follow up.

Filed as #1439

JoelEinbinder requested review from dgozman and pavelfeldman and removed request for dgozman February 29, 2020 00:37

JoelEinbinder force-pushed the d_ts branch from dff028f to 4ec47e7 Compare March 4, 2020 01:41

JoelEinbinder marked this pull request as ready for review March 4, 2020 01:43

dgozman reviewed Mar 4, 2020

View reviewed changes

JoelEinbinder mentioned this pull request Mar 5, 2020

[Feature] innerText not defined in evaluate context #1237

Closed

JoelEinbinder force-pushed the d_ts branch from 4ec47e7 to 9879817 Compare March 10, 2020 21:38

dgozman reviewed Mar 11, 2020

View reviewed changes

JoelEinbinder force-pushed the d_ts branch from a77a935 to 7b85635 Compare March 11, 2020 22:08

JoelEinbinder mentioned this pull request Mar 13, 2020

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

Merged

JoelEinbinder force-pushed the d_ts branch 2 times, most recently from 461e4dc to f5c458c Compare March 19, 2020 21:49

Joel Einbinder and others added 10 commits March 20, 2020 01:04

types: better types

631604f

a lot of fixes

462e231

easy fixes

20f5428

selectors and errors

43f3553

device descriptors

2d3c7d5

fix class comments when there is no toc

6c657b8

fixes

039e98d

rebase, timeouterrors

a3763b4

dont emit multiple payload interfaces, use arg name instead of options

a70f907

proper types for packages

fd02ed4

JoelEinbinder added 3 commits March 20, 2020 01:05

go back to interfaces, error namespace

98b718a

generate types on install, clean types on clean

5b16a76

spacing

81b1dc3

JoelEinbinder force-pushed the d_ts branch from 3b391fb to 81b1dc3 Compare March 20, 2020 00:06

fix install script and test types

72dbef9

dgozman approved these changes Mar 20, 2020

View reviewed changes

JoelEinbinder merged commit 825555c into microsoft:master Mar 20, 2020

This was referenced Mar 20, 2020

api(eval): accept zero or one arguments in all evaluation functions #1431

Merged

test(types): add test for types #1445

Merged

mxschmitt mentioned this pull request Mar 23, 2020

[BUG] No Playwright class typed definition anymore in @next #1483

Closed

JoelEinbinder mentioned this pull request Apr 8, 2020

[Feature] Overload waitForEvent to return specific type #1674

Closed

jharwig mentioned this pull request Apr 28, 2021

fix(types): fix waitForSelector typing to not union null when appropriate #6344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

types: better types #1166

types: better types #1166

JoelEinbinder commented Feb 29, 2020

JoelEinbinder commented Feb 29, 2020 •

edited

Loading

JoelEinbinder commented Mar 4, 2020

dgozman Mar 4, 2020

JoelEinbinder Mar 10, 2020

dgozman Mar 4, 2020

JoelEinbinder Mar 4, 2020

JoelEinbinder commented Mar 11, 2020

dgozman left a comment

dgozman Mar 11, 2020

JoelEinbinder Mar 11, 2020

JoelEinbinder commented Mar 11, 2020

dgozman commented Mar 12, 2020

JoelEinbinder commented Mar 20, 2020

dgozman commented Mar 20, 2020

dgozman left a comment

dgozman Mar 20, 2020

JoelEinbinder Mar 20, 2020

dgozman Mar 20, 2020

JoelEinbinder Mar 20, 2020

JoelEinbinder commented Mar 20, 2020

types: better types #1166

types: better types #1166

Conversation

JoelEinbinder commented Feb 29, 2020

JoelEinbinder commented Feb 29, 2020 • edited Loading

JoelEinbinder commented Mar 4, 2020

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 11, 2020

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

JoelEinbinder commented Mar 11, 2020

dgozman commented Mar 12, 2020

JoelEinbinder commented Mar 20, 2020

dgozman commented Mar 20, 2020

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

JoelEinbinder commented Mar 20, 2020

JoelEinbinder commented Feb 29, 2020 •

edited

Loading