add chromeFlags, chromePath and --headless as default #192
Conversation
gorangajic
commented
Aug 7, 2017
•
gorangajic
commented
- fixes Error: "PrintToPDF is not implemented" #146 pdf only works in headless mode
- fixes Pass chromePath and chromeFlags in Chromeless options to chrome-launcher #184 Add chromePath and chromeFlags to ChromelessOptions
|
@gorangajic Thank you for this PR. Would you mind adding |
| @@ -44,6 +44,7 @@ export default class Chromeless<T extends any> implements Promise<T> { | |||
| closeTab: true, | |||
| ...options.cdp, | |||
| }, | |||
| chromeFlags: ['--headless'], | |||
There was a problem hiding this comment.
Why not ['--headless', ...(options.chromeFlags||[])] ?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I also like the object hash option
gorangajic
force-pushed
the
add-headless-default
branch
from
30e0f2b to
21064a9
Compare
gorangajic
changed the title
|
@adieuadieu you had the most experience with the Chrome part. Please go ahead! |
|
Will chromeFlags support a |
|
@gorangajic do you know if this will allow proxies set in the flags to work on Lambda? |
|
@jborden13 not sure |
Fixes schickling#146 pdf only works in headless mode Fixes schickling#184 Add chromePath and chromeFlags to ChromelessOptions
felixfbecker
force-pushed
the
add-headless-default
branch
from
21064a9 to
deb8d40
Compare
Codecov Report
@@ Coverage Diff @@
## master #192 +/- ##
=======================================
Coverage 38.03% 38.03%
=======================================
Files 7 7
Lines 844 844
Branches 116 116
=======================================
Hits 321 321
Misses 523 523
Continue to review full report at Codecov.
|
|
What is waiting for this to be merged? |
