packages: adblocker-electron: conditionally enable webRequest handlers

[seg6]

We make heavy use of the webRequest APIs in our application, but the @ghostery/adblocker-electron package completely takes over the onHeadersReceived and onBeforeRequest handlers without providing an escape hatch, which makes it difficult for us to use the library.

This patch provides a way for users to manually drive the adblocking process from outside the @ghostery/adblocker-electron dependency.

[seia-soto]

Hi @nullptropy ,

Thanks for your PR.

I don't think the third argument for registering web request handlers optionally can be inferred clearly what actually it implies. Also, web request handlers are necessary for the library to provide an adblocking feature.

From my perspective, I'd rather prefer allowing web request handlers to be completely independent. For an instance, the following structure would give you a full access to the handler and optional registration of handlers.

// refs https://www.electronjs.org/docs/latest/api/web-request
mainWindow.webContents.session.webRequest.onHeadersReceived(filter, (details, callback) => {
  // Do some tasks prior to ad-blocking
  return blocker.handleHeadersReceived(details, callback);
})

Note that this is just another way to archive your needs. 👍

[seg6]

Hi @seia-soto! Thank you for the review.

I think that would be a better API to have, but my thinking was that:

• I need a way to make the library not register handlers with the webRequest API by default
• I didn't want to make a breaking change to the API where we'd not register handlers by default

How do you suggest we move forward with this?

[seia-soto]

Hi @nullptropy ,

If you really want to be flexible on those, I'd suggest you to completely remove adblocker-electron and use adblocker library directly. In one of our primary project: ghostery/ghostery-extension, we're directly using the adblocker library (not adblocker-webextension) to bring things as much as possible on manifest version 3 environment. Anytime, we (us and you) can backport adblocker co-packages instead.

Apart to my opinion, I'll ping my team to get a review on the redirection. Always thanks for your interest and investment to adblocker library.

Best

[chrmod]

@nullptropy whouldn't something like this work instead?

  const onBeforeRequest = blocker.onBeforeRequest.bind(blocker);
  blocker.onBeforeRequest = (...args) => {
    // run custom logic
    return onBeforeRequest(...args);
  };

The point of the adblocker-electron is to make it as easy to setup as possible. Once your requirements grow, you may want to consider dropping the dependency on the adblocker-electron completely, but maybe continue to use adblocker-electron-preload.

[seg6]

Thank you for the clarifications regarding the intent behind the @ghostery/adblocker-electron package.

I'd still argue that the default behavior of overwriting some webRequest handlers is not ideal API design, as the library implicitly changes application behaviour without clear documentation. I think documenting this behaviour and also offering an escape hatch would be the best way forward, as it's not uncommon for Electron projects to use the webRequest API.

Apart from this issue, @ghostery/adblocker-electron fits perfectly to our needs. Ideally, we would prefer to rely on an upstream implementation rather than maintaining our own fork/version, especially for such a minor issue.

Either way, the presented workarounds also work for what I'm trying to do. I'll be closing this PR, but please let me know if you have an alternative solution for the API. I'd be happy to tackle the implementation.

[chrmod]

Can definitely see your point. A move into this direction may indeed be beneficial for all consumers. However, introduction of such change is breaking the current API, so if we want it, we should probably offer the same solution for other supported platforms like puppeteer and playwright. This will probably require a major version bump.

If you don't mind, I would prefer to re-open the PR to remind us about this proposal.