search

brave / adblock-rust

Brave's Rust-based adblock engine
Mozilla Public License 2.0
1.46k stars 124 forks source link

Error handling for JS bindings #182

Open antonok-edm opened 3 years ago

antonok-edm commented 3 years ago

Most of the JS bindings currently will throw confusing error messages if invalid arguments are supplied (e.g. see https://github.com/cliqz-oss/adblocker/pull/2125). It should be possible to write error handling logic with more descriptive messages so that the API is easier to use.

denosaurtrain commented 2 years ago

As a dev looking at good first issue labels on this repo, I find the scope of this issue a bit unclear. Could someone familiar with the project (@antonok-edm?) clarify the intent? Some possible interpretations:

  1. extend the architecture to improve JS-specific error handling in general (if so, what's lacking?)
  2. change how JS-specific errors are displayed (if so, change in what way?)
  3. add hard-coded error messages to make specific error cases more clear (if so, which ones?)

Reading through the example, I gather OP is referring to this comment: https://github.com/ghostery/adblocker/pull/2125#issuecomment-896372761:

@antonok-edm adblock-rs crashes if you pass type as "ping"

Sorry, the value actually seems to be undefined

where the error message is given in the previous comment:

(node:48011) UnhandledPromiseRejectionWarning: TypeError: failed to downcast any to string
    at Engine.check (/adblocker/packages/adblocker-benchmarks/node_modules/adblock-rs/native/index.js:27:24)
    at Brave.match (/adblocker/packages/adblocker-benchmarks/blockers/brave.js:31:24)
    at main (/adblocker/packages/adblocker-benchmarks/run.js:264:26)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

Sure, it'd be awesome to have an Elm/Rust-style error message that uses ASCII art and colors to highlight the exact location of the type error and a suggestion for fixing it. However, for passing undefined to something that expected a string, the error message TypeError: failed to downcast any to string is reasonably descriptive, which leads me to believe I'm missing something. Maybe this error message comes up when loading these large request.json files, in which case the line number would be most useful? However, that's only if the scope of the issue is to improve error messages for loading these requests.json files, and I'm not confident that's the correct interpretation of "Most of the JS bindings currently will throw confusing error messages if invalid arguments are supplied."

antonok-edm commented 2 years ago

@denosaurtrain thanks for your interest in helping out!

TypeError: failed to downcast any to string

Maybe I'm just less comfortable with JS, but to me this error is really not useful and forces me to dig into the library to figure out what went wrong. It's not ideal to make users learn the implementation details of a library they've chosen, especially if those details are written in a different language!

At the very least, I'd like to see information about which argument couldn't be casted correctly, and probably also the type of the value that was passed. So, maybe something like this:

request_type argument expected string but received undefined

As for your interpretations here:

  1. extend the architecture to improve JS-specific error handling in general (if so, what's lacking?)
  2. change how JS-specific errors are displayed (if so, change in what way?)
  3. add hard-coded error messages to make specific error cases more clear (if so, which ones?)

Honestly, any of these approaches would be an improvement I'd be happy to see, if they're helping to move towards more descriptive messages like the example above. So far, I think all of the JS bindings suffer from similar issues.