New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(package): update addons-linter to version 0.26.1 #1082
Conversation
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. |
…ting errors on addons-linter 0.26.1)
@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:
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). |
@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:
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
|
Hmm, so gajus/eslint-plugin-flowtype#261 (comment) seems to workaround that for now. Maybe worth looking into that? |
@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 |
d6955f9
to
73809c3
Compare
73809c3
to
d1d9fb7
Compare
@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", |
d1d9fb7
to
ba556cc
Compare
ba556cc
to
dedd4be
Compare
@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):
How that sounds to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
tasks/configs/copy.js
Outdated
@@ -0,0 +1,11 @@ | |||
module.exports = { | |||
'create-artifacts-production': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a small nit but the full command would read better to me as copy:dist-files-to-artifacts-dir
.
tasks/configs/copy.js
Outdated
files: [ | ||
{ | ||
expand: true, | ||
src: ['package.json', 'dist/**', 'bin/**'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
dedd4be
to
e489ced
Compare
e489ced
to
0096668
Compare
Closes #1076
Here's an explanation of the problem. Eslint needs to load the
eslint-recommended
plugin. Before the latestaddons-linter
upgrade, it worked like this:This works fine because there's only one
eslint
in the tree:After upgrading
addons-linter
, we introduce a neweslint
and it loads a different version ofeslint-recommended
:Here's where you can see the conflicting packages:
Why is
eslint
loading the wrongeslint-recommended
? These bugs could be related but in our case it fails withnpm
, not justyarn
: