Opened 10 months ago

Closed 9 months ago

Last modified 5 months ago

#4864 closed change (fixed)

Start using ESLint for adblockpluschrome repository

Reported by: kzar Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-1.13.3-for-Chrome-Opera
Module: Platform Keywords:
Cc: sebastian, Ross, rraceanu, philll, sclow Blocked By: #3692
Blocking: #5080 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29374674/

Description (last modified by kzar)

Background

We've created an ESLint configuration for the company eslint-config-eyeo (see #3692). Now we need to make changes to adblockpluschrome to use ESLint and that configuration.

What to change

  • Add a ESLint configuration to adblockpluschrome which inherits from eslint-config-eyeo:
{
  "extends": "eslint-config-eyeo",
  "root": true,
  ...
}
  • Add an ESLint ignore file to ignore files such as jQuery.
  • Make the necessary changes to the adblockpluschrome code so that linting passes.
  • Add a note to the README explaining how to lint the code. (eslint *.js lib/ qunit/ ext/ chrome/)

Hints for testers

Unfortunately these changes were pretty comprehensive and will require very thorough testing of pretty much all code paths. Please take some time to test this on Chrome 49, a recent version such as 57 and Opera. I recommend testing this along side #5061 (and perhaps #5023) since that also contained large sweeping changes. Really anything and everything you can think of needs to be tested here.

Change History (16)

comment:1 Changed 10 months ago by kzar

  • Blocked By 4871 added

comment:2 Changed 10 months ago by kzar

  • Description modified (diff)

comment:3 Changed 10 months ago by kzar

  • Blocked By 4871 removed
  • Description modified (diff)

comment:4 Changed 9 months ago by kzar

  • Blocked By 5060 added

comment:5 Changed 9 months ago by kzar

  • Blocked By 5060 removed

comment:6 Changed 9 months ago by kzar

  • Blocked By 5077 added

comment:7 Changed 9 months ago by kzar

  • Blocked By 5079 added

comment:8 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 4864 - Start using ESLint for adblockpluschrome

comment:9 Changed 9 months ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next

comment:10 Changed 9 months ago by kzar

  • Blocked By 5079 removed

comment:11 Changed 9 months ago by kzar

  • Blocked By 5077 removed

comment:12 Changed 9 months ago by kzar

  • Cc Ross rraceanu philll sclow added
  • Description modified (diff)
  • Resolution set to fixed
  • Review URL(s) modified (diff)
  • Status changed from new to closed

I've opened #5080 for the changes required to reduce the /* globals ... */ comments. Originally I was going to reuse this one but I realised that was probably a mistake.

Phill / Ross / Robert / Scott - Heads up testing this, #5061 and #5023 will require a whole bunch of work. I recommend combining testing of those three and starting soon as possible. There is a high likelihood of regressions with all these changes, we don't want to miss them or find them just before the next release.

comment:13 Changed 9 months ago by sebastian

As for testing, note that the devbuilds on the Chrome Web Store don't get updated at the moment due to an issue on their end we are waiting for to get fixed, see ticket:5027#comment:25.

comment:14 Changed 9 months ago by kzar

  • Blocking 5080 added

comment:15 Changed 9 months ago by Ross

kzar / sebastian: Have picked this up.

comment:16 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. The build appears to working as expected across a variety of browser versions.

ABP 1.13.2.1785
Chrome 49 / 54 / 58 / Windows 7
Opera 36 / 42 / 45 / Windows 7

Note: See TracTickets for help on using tickets.