New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: improve TS types for launching browsers #6888
Conversation
src/node/LaunchOptions.ts
Outdated
* `puppeteer.launch` without having to list the set of all types. | ||
* @public | ||
*/ | ||
export type AllNodeLaunchOptions = BrowserArgOptions & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to find better names here because BrowserArgOptions
and BrowserOptions
are very similar.
I'm wondering about:
BrowserArgOptions
=>BrowserLaunchArgumentOptions
BrowserOptions
=>BrowserConnectOptions
To make it clearer that BrowserOptions
can be used when connecting to an existing browser (it's what puppeteer.connect
takes) and BrowserArgOptions
is only when launching browsers.
@mathiasbynens wdyt? If we have to make a breaking change for this we might as well try to make all the names useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BrowserLaunchArgumentOptions
+ BrowserConnectOptions
sgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Mathias that BrowserLaunchArgumentOptions
+ BrowserConnectOptions
looks reasonable.
LaunchOptions
and AllNodeLaunchOptions
have confusing names as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah these are confusing :( My hope is that users don't have to worry about the "sub types" and can just use the main type. I'd like to do a wider clean up of these types but they are used in a few places across the codebase and API so I'd rather keep the scope down for now and try to improve the names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
@sadym-chromium Please also take a look |
This commit tidies up the quite confusing state of all the various types required to launch a browser. As we saw when upgrading DevTools (https://source.chromium.org/chromium/chromium/src/+/master:third_party/devtools-frontend/src/test/conductor/hooks.ts;l=77), we had to define our launch options variable like so: ```ts const opts: puppeteer.LaunchOptions & puppeteer.ChromeArgOptions & puppeteer.BrowserOptions = { ... }; ``` This commit fixes that by introducing `AllNodeLaunchOptions`, which is defined as the intersection of all the above types. Additionally, the types defined as `ChromeArgOptions` are actually used for launching both Chrome and Firefox, so I have renamed them to `BrowserArgOptions`, and therefore this change is breaking because anyone using `ChromeArgOptions` in their types will need to switch. BREAKING CHANGE: renamed type `ChromeArgOptions` to `BrowserLaunchArgumentOptions` BREAKING CHANGE: renamed type `BrowserOptions` to `BrowserConnectOptions`
12a555f
to
52af8c0
Compare
This commit tidies up the quite confusing state of all the various types
required to launch a browser. As we saw when upgrading DevTools
(https://source.chromium.org/chromium/chromium/src/+/master:third_party/devtools-frontend/src/test/conductor/hooks.ts;l=77),
we had to define our launch options variable like so:
This commit fixes that by introducing
AllNodeLaunchOptions
, which isdefined as the intersection of all the above types.
Additionally, the types defined as
ChromeArgOptions
are actually usedfor launching both Chrome and Firefox, so I have renamed them to
BrowserArgOptions
, and therefore this change is breaking becauseanyone using
ChromeArgOptions
in their types will need to switch.BREAKING CHANGE: renamed type
ChromeArgOptions
toBrowserArgOptions