Skip to content
This repository has been archived by the owner on Nov 24, 2018. It is now read-only.

add chromeFlags, chromePath and --headless as default #192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add chromeFlags, chromePath and --headless as default #192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export default class Chromeless<T extends any> implements Promise<T> {
closeTab: true,
...options.cdp,
},
chromeFlags: ['--headless'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ['--headless', ...(options.chromeFlags||[])] ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what when someone does not want --headless mode, then it's not possible to override flags? Anyway I am open to that change because I will never want to run chrome without headless flag :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. We cannot force the use of --headless. We can set it as a default as @gorangajic has done, but a user needs to be able to omit it. There are valid use cases where you'd want to see what's happening in a window. Another approach would be to add a headless: true parameter to the constructor options which defaults to true, then add the headless flag based on that. In some ways I prefer this as its more explicit and the behavior is clearer (rather than, if you want to disable the headless flag, knowing to pass an empty array of chromeFlags.) @joelgriffith what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could go the route of passing in a hash of options:

{ headless: true } => '--headless'
{ remoteDebuggingPort: 1234 } => '--remote-debugging-port=1234'
{ headless: false, disableGpu: true } => '--disable-gpu'

This way you can splat new params into place, and still have defaults like headless

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remoteDebuggingPort is already behind cdp options and chrome is using port provided there so no need to duplicate that logic here. Whatever you guys want, I am open to changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also like the object hash option

}

const chrome = mergedOptions.remote
Expand Down
4 changes: 3 additions & 1 deletion src/chrome/local.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ export default class LocalChrome implements Chrome {
const { port } = this.options.cdp
this.chromeInstance = await launch({
logLevel: this.options.debug ? 'info' : 'silent',
port,
port: this.options.cdp.port,
chromeFlags: this.options.chromeFlags,
chromePath: this.options.chromePath,
})
const target = await CDP.New({
port,
Expand Down
2 changes: 2 additions & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ export interface ChromelessOptions {
launchChrome?: boolean // auto-launch chrome (local) `true`
cdp?: CDPOptions
remote?: RemoteOptions | boolean
chromeFlags?: Array<string> // ['--headless']
chromePath?: string
}

export interface Chrome {
Expand Down