Opened 7 months ago

Closed 5 months ago

Last modified 3 months ago

#7360 closed change (fixed)

Provide interface to access contents of subscriptions.json

Reported by: greiner Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords:
Cc: mjethani, sebastian, kzar, hfiguiere, jsonesen Blocked By: #7371
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/74

Description (last modified by mjethani)

Background

We're fetching, parsing and interpreting the contents of subscriptions.json (previously subscriptions.xml) in various places throughout our code (adblockpluschrome/subscriptionInit.js, adblockplusui/js/desktop-options.js, adblockplusui/mobile-options.js) and with ui#9, we'd do that again.

While we could share that code across adblockplusui or even adblockpluschrome, this seems to be something that's relevant for anyone who has Core as a dependency. Also, discouraging non-Core code from directly accessing subscriptions.json would allow Core to assert more control over how it's being used.

Further information

Currently implemented at:

What to change

Expose API for accessing information contained within subscriptions.json from outside Core

Introduce a new *recommendations() generator function in a new file lib/recommendations.js that yields Recommendation objects based on data/subscriptions.json.

Also add a test/recommendations.js.

Hints for testers

Please see the relevant dependency update ticket for adblockpluschrome (Platform).

Change History (23)

comment:1 Changed 7 months ago by sebastian

  • Cc kzar added

Also note that with manifest V3 we no longer have access to DOMParser in the background page but are limited to the APIs available in Service Workers (plus extension APIs). So it might be a good time to change the format of subscription.xml to JSON (which can still be parsed with built-in APIs available in background pages after manifest V3).

Version 0, edited 7 months ago by sebastian (next)

comment:2 Changed 7 months ago by mjethani

  • Cc hfiguiere jsonesen added

@kzar thanks for pointing out about DOMParser

Would it be enough for the three use cases to simply provide a subscriptions.json instead?

Also I am wondering if we should move the file to the data directory in adblockpluscore now.

comment:3 follow-up: Changed 7 months ago by hfiguiere

This involve changing the sitescripts to generate JSON instead of XML from the subscriptionlist (an issue should be filed). Moving it to the data directory would make sense.

Last edited 7 months ago by hfiguiere (previous) (diff)

comment:4 in reply to: ↑ 3 ; follow-up: Changed 7 months ago by sebastian

Replying to mjethani:

@kzar thanks for pointing out about DOMParser

That was me. ;)

Also I am wondering if we should move the file to the data directory in adblockpluscore now.

Agreed.

Replying to hfiguiere:

This involve changing the sitescripts to generate JSON instead of XML from the subscriptionlist (an issue should be filed). Moving it to the data directory would make sense.

Please correct me if I'm wrong, but IIRC that automation has been inactive for years. Either way, we should replace it with an npm script within adblockpluscore, and if necessary automate it through GitLab CI (it supports periodical jobs).

comment:5 in reply to: ↑ 4 Changed 7 months ago by hfiguiere

Replying to sebastian:

Please correct me if I'm wrong, but IIRC that automation has been inactive for years. Either way, we should replace it with an npm script within adblockpluscore, and if necessary automate it through GitLab CI (it supports periodical jobs).

I have been running these scripts each and every time I updated the .xml file.
But yes we can replace this with something less obscure.

comment:6 Changed 7 months ago by mjethani

Let's open a separate ticket for the work in the Automation module for generating a subscriptions.json in the data directory in adblockpluscore. Let's keep this one for any further work in adblockpluscore for providing an API over the subscriptions.json.

There are two options: adblockpluschrome and adblockplusui use the subscriptions.json file directly by require()'ing it, or adblockpluscore provides an API (e.g. subscriptions.query({prefixes: ["es"]})).

comment:7 Changed 7 months ago by sebastian

  • Blocked By 7371 added

comment:8 Changed 7 months ago by mjethani

  • Blocking 6977 added

comment:9 Changed 7 months ago by sebastian

  • Blocking 6977 removed

comment:10 Changed 5 months ago by mjethani

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:11 Changed 5 months ago by mjethani

This ticket is available to work on now (cc: @jsonesen).

comment:12 Changed 5 months ago by mjethani

Alternatively we might add a filter() function for iterables (#6434) and then the recommendations() function could just yield all recommendations, which can then be filtered like so:

for (let recommendation of filter(
  recommendations(),
  ({type, languages}) => ["ads", "circumvention"].includes(type) &&
                         languages.includes(Utils.appLocale.substring(0, 2))
))
{
  ...
}

This way we don't have to implement any filtering logic here.

Last edited 5 months ago by mjethani (previous) (diff)

comment:13 follow-up: Changed 5 months ago by greiner

Maybe it's just me and feel free to ignore but there appears to be quite a bit of redundancy in let recommendations = require("recommendations").recommendations(…);?

Presumably, as part of a different module, that generator name would be more fitting (e.g. require("data").recommendations()). Otherwise a name such as require("recommendations").query() could also be an option, in particular to describe what to do with the recommendations.

comment:14 follow-up: Changed 5 months ago by sebastian

I may have another suggestion. How about exposing metadata from subscriptions.json as properties on the Subscription object, and then add a new function to the subscriptionClasses module that returns an array/generator of all subscription objects for the filter list URLs given in subscriptions.json?

comment:15 in reply to: ↑ 13 Changed 5 months ago by mjethani

Replying to greiner:

Maybe it's just me and feel free to ignore but there appears to be quite a bit of redundancy in let recommendations = require("recommendations").recommendations(…);?

I was thinking you would import it like this:

let {recommendations} = require("recommendations");

This is similar to how various other modules are imported.

comment:16 in reply to: ↑ 14 Changed 5 months ago by mjethani

Replying to sebastian:

I may have another suggestion. How about exposing metadata from subscriptions.json as properties on the Subscription object, and then add a new function to the subscriptionClasses module that returns an array/generator of all subscription objects for the filter list URLs given in subscriptions.json?

That would be too much coupling and actually we are trying to go in the other direction of using more and more plain objects. In the end I see both lib/filterClasses.js and lib/subscriptionClasses.js going away.

comment:17 Changed 5 months ago by mjethani

  • Owner set to mjethani

comment:18 Changed 5 months ago by mjethani

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:19 Changed 5 months ago by mjethani

  • Description modified (diff)

comment:20 Changed 5 months ago by abpbot

A commit referencing this issue has landed:
Issue 7360 - Add recommendations module

comment:21 Changed 5 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Summary changed from Provide interface to access contents of subscriptions.xml to Provide interface to access contents of subscriptions.json

comment:22 Changed 5 months ago by mjethani

  • Description modified (diff)

comment:23 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Subscription behaviour still works as expected.

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Note: See TracTickets for help on using tickets.