rubycdp / ferrum

Headless Chrome Ruby API
https://ferrum.rubycdp.com
MIT License
1.71k stars 123 forks source link

Improving browser options semantics #258

Closed gbaptista closed 8 months ago

gbaptista commented 2 years ago

Today, to set a browser option, you need to:

Ferrum::Browser.new(browser_options: { 'no-sandbox': nil })

This semantics can be misleading because, at least from my perspective, nil is associated with "absence."

It may also mislead people into trying to remove a default option by setting it as nil:

Ferrum::Browser.new(browser_options: { 'disable-extensions': nil })

This PR proposes the use of true for desired flags:

Ferrum::Browser.new(browser_options: { 'no-sandbox': true })

And the use of nil or false to allow the possibility of easily removing a default option:

Ferrum::Browser.new(browser_options: { 'disable-extensions': false })
Ferrum::Browser.new(browser_options: { 'disable-extensions': nil })

There's also protection to prevent people from removing required flags:

Ferrum::Browser.new(browser_options: { 'remote-debugging-port' => nil })
# => #<ArgumentError: remote-debugging-port is required>

I choose an explicit error if the person explicitly tries to remove a required flag rather than just overriding it with the required value. It could confuse the person not understanding why the removal didn't work.

Also, README was updated to instruct the new semantics.

Overall

Upsides:

Downsides:

route commented 2 years ago

I'm afraid it changes existing behaviour and there's no a single deprecation warning, I'll get back to it later.

route commented 8 months ago

Since it's a breaking change and there are no deprecation warnings, it's not a suitable solution for now. For a command line flag which in general can be --flag --flag=value or -flag -flag value hash fits just well. Having a key with nil value mirrors --flag, having a key with some value mirrors --flag=value, absence of k,v is absence of flag. It's counter-intuitive to pass nil, false and expect a flag to be removed instead of --flag=false in my opinion.

We should keep using current approach especially when it's been in use for years, or replace hash with something more complex, it needs research, but maybe something like this:

  options = Chrome::Options.new
  options.add_argument('--ignore-certificate-errors')
  options.add_argument('--disable-popup-blocking')
  options.add_argument('--disable-translate')