Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4552 closed change (fixed)

Drop dependency on JSHydra

Reported by: trev Assignee: kzar
Priority: P3 Milestone: Adblock-Plus-1.13-for-Chrome-Opera
Module: Automation Keywords:
Cc: sebastian, kzar, kvas, Ross, scheer, rraceanu Blocked By: #4551
Blocking: #4678 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Rraceanu Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29363565/
https://codereview.adblockplus.org/29363560/

Description (last modified by kzar)

Background

Transpiling JavaScript is no longer an essential part of our build infrastructure. With V8 supporting all the necessary functionality, neither Chrome/Opera nor libadblockplus will need transpiling any longer. The only platform still needing it is Safari which is considered legacy now.

What to change

Buildtools:

Remove JSHydra dependency from buildtools. packagerChrome.convertJS() should stay but merely as a way to combine modules into a single file. We don't need an external tool for that, this can be done from Python. While at it, we can choose a format that will only execute modules when they are required for the first time, along these lines (inspired by webpack):

let require = (function(modules)
{
  let cache = {};

  function require(name)
  {
    if (!cache.hasOwnProperty(name))
    {
      let module = {exports: {}};
      cache[name] = modules[name](module, module.exports, require);
    }
    return cache[name];
  }

  return require;
})
({
  "foo": function(module, exports, require)
  {
    // Contents of lib/foo.js go here
  },
  "bar": function(module, exports, require)
  {
    // Contents of lib/bar.js go here
  }
});

adblockpluschrome:

  • Update the buildtools dependency in adblockpluschrome to d8b095c772cb which will also include two unrelated changes.
  • Adjust require implementation in lib/compat.js.
  • Ensure everything works.

Hints for testers

As far as possible test all Adblock Plus functionality in an old (49) and modern (>=54) version of Chrome. Bugs could have appeared basically anywhere since this is a fairly large change which could have changed any of the code subtlety at build time.

Ensure that everything works correctly. (Make sure to check that filter + preference saving and loading works correctly since we made a small change there.)

Change History (20)

comment:1 Changed 3 years ago by trev

  • Component changed from Unknown to Build-and-Release-Tools

comment:2 Changed 3 years ago by kzar

  • Blocked By 4551 added
  • Cc sebastian kzar added
  • Priority changed from Unknown to P3
  • Ready set

comment:3 Changed 3 years ago by kzar

  • Owner set to kzar

comment:4 Changed 3 years ago by kzar

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

comment:5 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4552 - Drop jshydra dependency

comment:6 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4552 - Drop jshydra dependency

comment:7 Changed 3 years ago by kzar

  • Blocking 4678 added

comment:8 Changed 3 years ago by kzar

  • Cc Ross scheer rraceanu added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

(Sorry I should have created a separate issue for the dependency update. It's kind of messy having a platform milestone set to a buildtools issue.)

Adding you as Cc Robert+Ross+Scott as a heads up, this one needs a lot of testing. I've been personally running with the change for the last week or so without problems but it could have caused subtle regressions pretty much anywhere...

comment:9 Changed 3 years ago by kzar

Extremely stupidly I didn't test the build on Chrome 41 myself. It doesn't work. I've opened issues #4722 and #4723 for changes we'll need to do. Just setting up a chroot for Chrome 45 so I can ensure everything is working this time.

I am a huge idiot.

comment:10 Changed 3 years ago by kzar

Further testing showed that Chrome 45 was not good enough since it doesn't support destructuring. It looks like our choices are to either revert the changes to remove JS Hydra or to update the minimum Chrome version to 49.

I will send an email around now to see if dropping <49 support is acceptable.

comment:11 Changed 3 years ago by kzar

(So we've looked at the data and the consensus is to increase the minimum Chrome version to 49. I will do that now in issue #4722 and we will keep these changes to remove JS Hydra.)

comment:12 Changed 3 years ago by kzar

  • Description modified (diff)

comment:13 Changed 3 years ago by rraceanu

Verified on Windows 7 x32/64, Windows 8 x32/64, Windows 10 64, OSx 10.9/10.10/10.11 on Chrome 49-56 and Opera 39-42. I've encountered no new Issues when performing full round of testing with ABP 1.12.4

comment:14 Changed 3 years ago by kzar

1.12.4 is the current stable release which came out before this change. I think you should be testing with the latest devbuild which is 1.12.4.1738.

comment:15 Changed 3 years ago by rraceanu

I will postpone the testing for next release then since its on 14th of March in 2 weeks and will do it on 1.13

comment:16 Changed 3 years ago by kzar

Well the idea of testing is to catch bugs before the release. If you test with 1.13 after it comes out it will be too late.

Also once you have tested a ticket you need to assign yourself as the tester and mark "Verified working" as yes.

comment:17 Changed 3 years ago by rraceanu

I will resume testing starting tomorrow with the latest devbuild, will post the results within a week.

comment:18 Changed 3 years ago by rraceanu

  • Tester changed from Unknown to Rraceanu

comment:19 Changed 3 years ago by rraceanu

Verified on Windows 7 x32/64, Windows 8 x32/64, Windows 10 64, OSx 10.9/10.10/10.11 on Chrome 49-56 and Opera 39-42. I've encountered no new Issues with ABP 1.12.4.1739

comment:20 Changed 3 years ago by rraceanu

  • Verified working set
Note: See TracTickets for help on using tickets.