ferrum icon indicating copy to clipboard operation
ferrum copied to clipboard

Improving browser options semantics

Open gbaptista opened this issue 3 years ago • 1 comments

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:

  • A more intuitive way to work with flags.
  • Possibility to easier remove default flags.
  • Protection against removing required flags.
  • Potentially fewer issues and frustration compared to the previous way.

Downsides:

  • As today, people use nil to define a flag, so we may consider this a potential breaking change.

gbaptista avatar Apr 15 '22 01:04 gbaptista

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

route avatar Apr 18 '22 08:04 route

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')

route avatar Jan 06 '24 11:01 route