add chromeFlags, chromePath and --headless as default#192
add chromeFlags, chromePath and --headless as default#192gorangajic wants to merge 1 commit intoschickling:masterfrom
Conversation
| @gorangajic Thank you for this PR. Would you mind adding |
| 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? |
10 participants
Uh oh!
There was an error while loading. Please reload this page.