Opened 22 months ago

Closed 22 months ago

Last modified 21 months ago

#5429 closed defect (fixed)

ESLint v4.0.0 breaks eslint-config-eyeo

Reported by: mjethani Assignee: kzar
Priority: Unknown Milestone:
Module: Sitescripts Keywords:
Cc: kvas, sebastian, kzar, hfiguiere Blocked By:
Blocking: #5449 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29492585/
https://codereview.adblockplus.org/29496566/

Description (last modified by mjethani)

Environment

ESLint v4.0.0
adblockpluschrome repo at changeset 7aeb2fb83e11
eslint-config-eyeo 1.0.4

How to reproduce

  1. Run the command eslint *.js lib/ qunit/ ext/ chrome/ from the repo

Observed behaviour

104 errors.

Expected behaviour

No errors.

Notes

It seems there have been major breaking changes in ESLint 4.0.0. Either we update the config and fix our code where appropriate, or we stick with the v3.x.x.

Change History (20)

comment:1 Changed 22 months ago by mjethani

  • Cc kvas sebastian kzar added

comment:2 Changed 22 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 22 months ago by mjethani

I was able to fix many of the errors by adding the following to my .eslintrc.json:

"rules": {
  "indent": [
    "error", 2, {
      "SwitchCase": 1,
      "ArrayExpression": "first",
      "FunctionDeclaration": {"parameters": "first"},
      "FunctionExpression": {"parameters": "first"},
      "CallExpression": {"arguments": "off"},
      "MemberExpression": "off",
      "ObjectExpression": "first"
    }
  ],
  "no-multi-spaces": ["error", {"ignoreEOLComments": true}]
}

For the rest, I had to make changes to the code as there's no good way to disable those checks.

comment:4 Changed 22 months ago by mjethani

In particular ESLint 4.0.0 doesn't like this:

const {Export1, Export2, Export3,
       Export4, Export5} = require("example");

We have to change it to one of the following two:

const {Export1, Export2, Export3, Export4, Export5} =
  require("example");
const {
  Export1, Export2, Export3,
  Export4, Export5
} = require("example");

Also it doesn't like this:

return value1 &&
       (flag ?
          value2 :
          value3);

We have to change it to:

return value1 &&
       (
         flag ?
           value2 :
           value3
       );
Last edited 22 months ago by mjethani (previous) (diff)

comment:5 Changed 22 months ago by mjethani

As part of fixing this issue, we should probably update the value of "eslint" in "peerDependencies" to just "4". This will allow for only minor and patch level changes (i.e. non-breaking). With the latest version of NPM it's possible to install modules locally and run them easily from the command line.

comment:6 Changed 22 months ago by mjethani

  • Review URL(s) modified (diff)

comment:7 Changed 22 months ago by mjethani

The workaround for development purposes for now is to use npm install -g eslint@3.19 eslint-config-eyeo to install version 3.19 of ESLint along with the config.

In addition, we should add "ObjectExpression": "first" to fix the one error that was introduced in adblockpluschrome with changeset 0259a924a1af.

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

comment:8 Changed 22 months ago by sebastian

This seems like a regression bug in ESLint to me, in particular in those cases where there is no option we could add to our configuration in order to restore the previous behavior. We should file issues with upstream ESLint, and stick with ESLint 3.19 for the time being, IMO. I'd rather not perform random changes to our code, some of which might even degrade readability.

comment:9 Changed 22 months ago by mjethani

  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:10 Changed 22 months ago by hfiguiere

  • Cc hfiguiere added

comment:11 Changed 22 months ago by mjethani

I have filed two bugs in ESLint: #8977, #8979.

Meanwhile, we have the option of using indent-legacy if we want to upgrade to ESLint 4 without making any code changes. Alternatively, we could stick with ESLint 3.19. The third option, of course, is to update the configuration and make minor changes to the code.

comment:12 Changed 22 months ago by mjethani

Well, it looks like the second bug is fixed in the next release, which happily is today. This leaves us with the option of configuration updates and one small change to our code.

Note that there is another change but that's a genuine error that we should fix.

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

comment:13 Changed 22 months ago by mjethani

ESLint 4.3.0 is out.

I've confirmed that ObjectExpression (#8979) is fixed in this release.

comment:14 Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 5429 - Upgrade to ESLint 4

comment:15 Changed 22 months ago by abpbot

A commit referencing this issue has landed:
Issue 5429 - Release eslint-config-eyeo 2.0.0

comment:16 Changed 22 months ago by kzar

  • Owner set to kzar
  • Review URL(s) modified (diff)

Since much of the team are away I have made the minimum configuration changes required to have ESLint 4 behave like ESLint 3 for now. (I've switched from indent to indent-legacy and added the ignoreEOLComments option to no-multi-spaces.)

I've gone with Manish's suggestion of specifying ESLint 4 now, so when ESLint 5 is released we won't be upgraded accidentally. I've also gone with his suggestion of updating the eslint-config-eyeo version to 2.0.0 since the indent-legacy option is not supported for older version of ESLint and so this is a breaking change.

I will now file an issue where we can continue the discussion about indentation for our ESLint configuration and our coding style guide.

comment:17 Changed 22 months ago by mjethani

Confirmed, this works on adblockpluschrome, adblockplusui, and adblockpluscore master with eslint-config-eyeo 2.0.0 and ESLint 4.

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

comment:18 Changed 22 months ago by kzar

  • Blocking 5449 added

comment:19 Changed 22 months ago by kzar

  • Resolution set to fixed
  • Status changed from new to closed

I've created #5449 for the discussion and further work required regarding indentation.

comment:20 Changed 21 months ago by mjethani

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.