Opened on 10/20/2016 at 08:12:35 AM
Closed on 11/30/2016 at 04:24:17 PM
Last modified on 03/14/2017 at 06:15:32 AM
#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/ |
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.)
Attachments (0)
Change History (20)
comment:1 Changed on 10/20/2016 at 08:12:54 AM by trev
- Component changed from Unknown to Build-and-Release-Tools
comment:2 Changed on 10/20/2016 at 08:51:23 AM by kzar
- Blocked By 4551 added
- Cc sebastian kzar added
- Priority changed from Unknown to P3
- Ready set
comment:3 Changed on 11/17/2016 at 04:35:59 PM by kzar
- Owner set to kzar
comment:4 Changed on 11/18/2016 at 05:27:56 PM by kzar
- Cc kvas added
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:5 Changed on 11/30/2016 at 04:02:23 PM by abpbot
comment:6 Changed on 11/30/2016 at 04:08:02 PM by abpbot
A commit referencing this issue has landed:
Issue 4552 - Drop jshydra dependency
comment:7 Changed on 11/30/2016 at 04:12:30 PM by kzar
- Blocking 4678 added
comment:8 Changed on 11/30/2016 at 04:24:17 PM 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 on 12/09/2016 at 11:27:37 AM by kzar
comment:10 Changed on 12/09/2016 at 01:58:28 PM 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 on 12/13/2016 at 10:44:16 AM 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 on 12/16/2016 at 01:10:36 PM by kzar
- Description modified (diff)
comment:13 Changed on 03/01/2017 at 02:07:48 AM 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 on 03/01/2017 at 02:20:37 AM 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 on 03/01/2017 at 02:49:19 AM 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 on 03/01/2017 at 03:48:13 AM 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 on 03/01/2017 at 04:24:47 AM by rraceanu
I will resume testing starting tomorrow with the latest devbuild, will post the results within a week.
comment:18 Changed on 03/01/2017 at 04:24:57 AM by rraceanu
- Tester changed from Unknown to Rraceanu
comment:19 Changed on 03/14/2017 at 06:15:15 AM 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 on 03/14/2017 at 06:15:32 AM by rraceanu
- Verified working set
A commit referencing this issue has landed:
Issue 4552 - Drop jshydra dependency