Opened 4 years ago

Closed 2 years ago

#3518 closed change (rejected)

Use fetch API instead XMLHttpRequest

Reported by: sebastian Assignee:
Priority: Unknown Milestone:
Module: Adblock-Plus-for-Firefox Keywords:
Cc: trev Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

The fetch() API is a more modern interface, with a more streamlined API than XMLHttpRequest, to do essentially the same thing, sending HTTP requests with JavaScript. Most notably, the new API is based on promises, which make asynchronous code easier to write and read. But it is also a little more powerful; so for example fetch() let's you specify the caching strategy, which we so far rely on non-standard properties for that are only supported on Firefox.

With #3515 we will have a polyfill in place for older Chrome/Opera versions as well as Safari, which don't have native support for fetch(). So it can even be used in the core code which is shared cross-platform.

What to change

Replace all usage of XMLHttpRequest with the fetch() API.

Change History (5)

comment:1 follow-up: Changed 4 years ago by trev

There are significant issues with this as far as Firefox is concerned. Fetch API is only supported starting with Firefox 39 (we are stuck supporting Firefox 38 for at least a few more months). Worse yet, originally it was only supported on windows - we are running in a sandbox and support was only added there in Firefox 41. And of course the sandbox created for bootstrapped add-ons doesn't have Fetch API exposed on it. So we would either have to sandbox our modules (will likely degrade performance) or create a sandbox just for the purpose of getting the Fetch API out of it (should be preferable but I wonder whether it will work outside of that sandbox just like that).

Altogether, I think that this is premature - we should wait until we can drop support for everything before Firefox 41 rather than messing with polyfills now and making this unnecessarily complicated.

Note that libadblockplus will have to be adjusted as well if we do that, it currently implements a fake XMLHttpRequest object. I wonder whether V8 version used there is recent enough to suppport promises natively...

comment:2 Changed 4 years ago by sebastian

  • Status changed from new to reviewing

comment:3 Changed 4 years ago by sebastian

  • Status changed from reviewing to reopened

comment:4 in reply to: ↑ 1 Changed 4 years ago by sebastian

Replying to trev:

There are significant issues with this as far as Firefox is concerned. Fetch API is only supported starting with Firefox 39 (we are stuck supporting Firefox 38 for at least a few more months). Worse yet, originally it was only supported on windows - we are running in a sandbox and support was only added there in Firefox 41. And of course the sandbox created for bootstrapped add-ons doesn't have Fetch API exposed on it. So we would either have to sandbox our modules (will likely degrade performance) or create a sandbox just for the purpose of getting the Fetch API out of it (should be preferable but I wonder whether it will work outside of that sandbox just like that).

Altogether, I think that this is premature - we should wait until we can drop support for everything before Firefox 41 rather than messing with polyfills now and making this unnecessarily complicated.

Ouch, I hoped we could simply drop support for Firefox 38, or migrate to fetch() once we did so. But also considering the limitation on FF <41, I'd rather go with a polyfill. Note that polyfilling just the subset of the Fetch API we need, as we already do for old Chrome and Safari version, is quite trivial. Also note that half of the code in that polyfill would be duplicated anyway when using XMLHttpRequest directly. So if you don't like polyfills, just consider it a convenience wrapper. ;)

Note that libadblockplus will have to be adjusted as well if we do that, it currently implements a fake XMLHttpRequest object. I wonder whether V8 version used there is recent enough to suppport promises natively...

Well, if we use a polyfill in the core code, things would keep working for the time being. But yeah, sooner or later we would have to replace the fake XMLHttpRequest object there with a fake fetch() function.

comment:5 Changed 2 years ago by trev

  • Resolution set to rejected
  • Status changed from reopened to closed

Mass-closing all bugs in Adblock Plus for Firefox module, the codebase of Adblock Plus 3.0 belongs into Platform and User-Interface modules. Old bugs are unlikely to still apply.

Note: See TracTickets for help on using tickets.