Opened 16 months ago

Closed 16 months ago

Last modified 12 months ago

#7449 closed change (fixed)

Move compareVersion function into separate module and export

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

Description (last modified by mjethani)


To implement a workaround for #7430 I would like to emit an event, but only for users running Firefox versions 66 or 67. To do that I need a way to compare info.platformVersion to those numbers. This logic has already been implemented in adblockpluscore/lib/notification.js, but is not exported for use elsewhere.

What to change

  • Move the compareVersion function into its own module lib/versions.js, rename it to compareVersions and export.
  • Add unit tests for the compareVersions function.
  • Import that function from lib/notification.js and adjust the calling code to use the new name.

Hints for testers

Set up a notification.json on your test server:

  "notifications": [
      "type": "critical",
      "title": "Hello",
      "message": "I just want to say hello.",
      "targets": [
          "applicationMaxVersion": "75.0.3759.4"
  "version": "201904101149"

(Note: I'm not sure how you normally test notifications, but I imagine you redirect the request to to your own server somehow.)

The value of applicationMaxVersion should be set to be greater than or equal to the version number you see in the About dialog of your browser.

Now do a fresh install of the extension, wait for a minute, and see that you get notified with the message I just want to say hello.

Next, edit the notification.json to make the value of applicationMaxVersion less than your browser's version number; then do a fresh install of the extension again, wait for a minute, and see that you do not get notified with the same message.

This should be enough, but you could also experiment with applicationMinVersion in the same way if you like, try different values (less than the browser version, greater than the browser version, exactly equal to the browser version), values with only the major version number, values with only the major and minor version numbers, and so on.

It would be a good idea to test this on all supported platforms since the formats of the version numbers may differ.

Change History (14)

comment:1 Changed 16 months ago by mjethani

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 16 months ago by mjethani

I wouldn't mind if this went into its own lib/versions.js module with an accompanying test/versions.js test file.

While we're at it, maybe it's a good opportunity to rename the function to compareVersions() since it actually compares two versions against each other.

comment:3 Changed 16 months ago by mjethani

  • Cc hfiguiere added

comment:4 Changed 16 months ago by kzar

  • Description modified (diff)
  • Keywords goodfirstbug added
  • Summary changed from Move compareVersion function into coreUtils module and export to Move compareVersion function into separate module and export

Sounds good to me, updated the description.

comment:5 Changed 16 months ago by kzar

  • Description modified (diff)

comment:6 Changed 16 months ago by mjethani

  • Owner set to mjethani

comment:7 Changed 16 months ago by mjethani

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

comment:8 Changed 16 months ago by sebastian

Note that the compareVersion() function were previously part of the public API, but we stopped exporting it, in order to simplify things. I don't think that at the time we need it for anything outside of the notifications module.

For the workaround mentioned (#7430) where we want to target only specific Firefox versions, we will be just using parseInt() which is good enough when only checking for the major version.

comment:9 Changed 16 months ago by mjethani

Does this mean that #7430 does not depend on this change?

comment:10 Changed 16 months ago by kzar

Yea, since we could go ahead with #7430 without this change I didn't mark this issue as blocking it and I added the keyword "goodfirstbug" in case a new contributor wanted to pick it up.

Anyway, you already did a pretty good job of exporting the function and I'm in favour of the change. Even if we don't use it for #7430 it's the kind of function that will surely be useful in the future IMO. Even better now that you've added a bunch of great tests too, surely this is a good thing?

comment:11 Changed 16 months ago by mjethani

Yes, I agree it's a good idea to move the function to its own module and add some tests. For reference, we might even need it for snippets (#7451).

comment:12 Changed 16 months ago by abpbot

A commit referencing this issue has landed:
Issue 7449 - Move compareVersion() to lib/versions.js

comment:13 Changed 16 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:14 Changed 12 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described. Tested with a variety of targets and version numbers.

Microsoft Edge 44.17763.1.0 / Windows 10 1809

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

Note: See TracTickets for help on using tickets.