fix(package): update addons-linter to version 0.26.1

[kumar303]

Closes #1076

Here's an explanation of the problem. Eslint needs to load the eslint-recommended plugin. Before the latest addons-linter upgrade, it worked like this:

$ eslint --debug 'src/**/*.js'
...
  eslint:config-file Loading /Users/kumar/dev/web-ext/node_modules/eslint/conf/eslint-recommended.js +7ms

This works fine because there's only one eslint in the tree:

$ yarn list eslint
yarn list v1.1.0
└─ eslint@3.19.0

After upgrading addons-linter, we introduce a new eslint and it loads a different version of eslint-recommended:

$ eslint --debug 'src/**/*.js'
...
  eslint:config-file Loading /Users/kumar/dev/web-ext/node_modules/addons-linter/node_modules/eslint/conf/eslint-recommended.js +4ms

Here's where you can see the conflicting packages:

$ yarn list eslint
yarn list v1.1.0
├─ addons-linter@0.26.1
│  └─ eslint@4.7.1
└─ eslint@3.19.0

Why is eslint loading the wrong eslint-recommended? These bugs could be related but in our case it fails with npm, not just yarn:

• ESLint plugin issue when using yarn (but works w/ NPM) eslint/eslint#8547
• 'eslint --init' installs wrong versions of depencies for AirBnB eslint/eslint#7338

[EnTeQuAk]

It seems that his is hellarously broken. I wonder if it's easier for web-ext to upgrade to eslint 4.x? This isn't a great situation but sounds like a reasonable workaround to unblock any new release.

[coveralls]

Coverage remained the same at 100.0% when pulling 88b4ead on greenkeeper/addons-linter-0.26.1 into 436e26d on master.

[rpl]

@kumar303 @EnTeQuAk the change in 88b4ead adds an explicit devDependencies between web-ext and eslint 3.19.0, this is enough to fix the unexpected eslint linting errors

Unfortunately it also unveils another issue:

• addons-linter has a strong dependency on eslint 4, because it is needed for more than just linting the addons-linter sources, it is part of the dependencies that addons-linter is going to use at runtime

• web-ext is currently (loosely) depending from eslint 3, by adding it explicitly to the web-ext devDependencies, we ensure that (in a developer installation) the web-ext sources will be linted using the expected eslint version

• when web-ext runs the functional tests on travis, the "node_modules/" dir has been created by including the web-ext devDependencies (which is not true when web-ext is installed by a user or its dependencies installed using "npm install --production"), and so the eslint version that the addons-linter is going to use is eslint 3.19.0 instead of the eslint 4.x expected by updated addons-linter sources and the change in mozilla/addons-linter@9d4f45b#diff-2bf56d6974b3fd35628de87b7c500747R75 doesn't work on eslint 3

Locally I've manually verified that in a user installation "web-ext lint" is going to use eslint 4 as expected and so the linting command works correctly, nevertheless on travis the related test is going to fail because of eslint 3 used at runtime (which to be fair is a limitation/issue of the functional tests which should really use a node_modules/ dir generated as it would be with "npm install --production", to prevent this issue but also to be able to catch other issue related to differences in the "node_modules/" dir content between development and production installations).

[rpl]

@kumar303 @EnTeQuAk I've been looking more deeply into the eslint errors that are produced when the web-ext sources are linted using eslint-4 (e.g. to be able to evaluate the alternative strategy suggested by @EnTeQuAk in his comment and upgrade eslint-4 in web-ext) and here is some additional details about the reason behind the unexpected linting errors:

e.g. if we look into the following two linting errors:

.../web-ext/src/util/updates.js
  13:6  error  'CheckForUpdatesParams' is already declared in the upper scope  no-shadow

.../web-ext/src/watcher.js
  30:13  error  'OnSourceChangeFn' is not defined                                no-undef

The immediate thing that I notice is that they are both related to flow annotations, which should not trigger these linting errors, iirc it is eslint-plugin-flowtype that should prevent eslint to wrongly report flow annotations as no-undef or no-shadow linting errors and it looks that it is not working correctly on eslint-4, e.g.:

• Flow generics trigger no-shadow rule gajus/eslint-plugin-flowtype#275 - Flow generics trigger no-shadow rule
• Eslint v4 - no-undef not suppressed gajus/eslint-plugin-flowtype#261 - Eslint v4 - no-undef not suppressed

[EnTeQuAk]

Hmm, so gajus/eslint-plugin-flowtype#261 (comment) seems to workaround that for now. Maybe worth looking into that?

[rpl]

@EnTeQuAk unfortunately manually declaring all our flow types as globals doesn't seem a be great options for us (in that case the reporter seems to have issues only on a restricted number of flow types which are defined internally by flow, in our case eslint is complaining about the flow types that we are declaring in the web-ext sources itself)

and we have also the issue with the no-shadow rule :-(

[coveralls]

Coverage remained the same at 100.0% when pulling 73809c3 on greenkeeper/addons-linter-0.26.1 into 177709b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling 73809c3 on greenkeeper/addons-linter-0.26.1 into 177709b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling d1d9fb7 on greenkeeper/addons-linter-0.26.1 into 177709b on master.

[rpl]

@kumar303 @EnTeQuAk the patch in d1d9fb7 is a (super gross) way to show what I've observed locally and it is still not "the workaround that we can use in the meantime" (installing an exact version of eslint manually in the travis and appveyor files is not something that we can really use as a workaround).

The best workaround/solution (at least in my current vision) would be to be able to run the functional tests on a web-ext build that is using the "node_modules/" dir created using "npm install --production",
e.g. because besides fixing the pressing problem of the two different eslint version that we would need to use right now, it will also allow to catch other similar issues related to the different "node_modules/" layout that is created by "npm install" (developer) vs. "npm install --production" (user).

[coveralls]

Coverage remained the same at 100.0% when pulling ba556cc on greenkeeper/addons-linter-0.26.1 into 177709b on master.

[coveralls]

Coverage remained the same at 100.0% when pulling dedd4be on greenkeeper/addons-linter-0.26.1 into 177709b on master.

[rpl]

@kumar303 @EnTeQuAk In ba556cc there is a prototype of the approach described in my previous comment (and it works on both travis and appveyor, as well as locally):

• using a grunt task we create a clone of the web-ext dir with just the "package.json", the bundled files from "dist/" and the binary file form "bin/"
• in .travis.yml and appveyor.yml the npm packages in the cloned "artifacts/production" dir are installed using "npm install --production"
• then we point the functional tests to the web-ext executable in "artifacts/production/bin/" (which is going to use exactly the same dependencies that a user is going to use)

How that sounds to you?

[kumar303]

I like it! I only had minor suggestions but let's merge this ASAP.

The other issue you raised about developers who might have trouble with web-ext lint is interesting but we should tackle that separately.

[kumar303]

Just a small nit but the full command would read better to me as copy:dist-files-to-artifacts-dir.

[kumar303]

Is there an easy way to make this throw an error if dist is empty? A simple message like 'run the build command first' might prevent some confusion.

[kumar303]

I looked at grunt-contrib-copy but I don't see anything obvious. It's not worth spending too much time on. Maybe just add a comment to this file that you need to build first?

[coveralls]

Coverage remained the same at 100.0% when pulling 0096668 on greenkeeper/addons-linter-0.26.1 into 177709b on master.