-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add support for eslint 5 #1843
Add support for eslint 5 #1843
Conversation
The test failure also happens on master and is due to babel-eslint@8.2.4. Downgrading to 8.2.3 fixes it.
Perhaps the easiest fix is to just drop node 4 support now that everyone else in the eslint ecosystem is doing it. |
@papandreou just released babel-eslint@8.2.5, which reverts the babel 7-b51 bump. |
a83804c
to
3cf7fec
Compare
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.
Please also add testing for eslint 4 vs 5 here: https://github.com/yannickcr/eslint-plugin-react/blob/master/.travis.yml
Also, please make the dev dep range for eslint identical to the peer dep range.
@ljharb, thanks for the quick response!
Okay, do you mean like this? d717068 ... I guess the exact configuration depends on your answer to the below question :)
I tried that initially, but it makes the build fail in node 4: https://travis-ci.org/yannickcr/eslint-plugin-react/jobs/395825658#L856 Are you ready to part with node 4 support -- seems like everyone else is :) |
.travis.yml
Outdated
- node_js: '7' | ||
env: TEST=true ESLINT=5 | ||
- node_js: '6' | ||
env: TEST=true ESLINT=5 |
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.
sorry for being unclear :-) now we’re not testing eslint 4. Can you change al the “next”s to “4”s?
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.
Sure, fixed in d0bebd7 :)
As for node 4, no, we should not drop support for it (nobody should). I’ll look into the failing tests. |
@ljharb, thanks a lot! I agree, but eslint 5 dropped node 4 support: https://eslint.org/docs/5.0.0/user-guide/migrating-to-5.0.0#drop-node-4 Would be great for this project to maintain backwards compatibility via older eslint versions, of course. |
.travis.yml
Outdated
@@ -47,4 +55,4 @@ matrix: | |||
- node_js: '9' | |||
- node_js: '7' | |||
- node_js: '5' | |||
- env: TEST=true ESLINT=next | |||
- env: TEST=true ESLINT=4 |
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.
let’s remove this line
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.
Whatever you say, fixed in 61372b8 :)
ahhh well true, in that case for eslint 5 only, we should definitely not be testing on node 4. |
Sounds like the right way to handle it. |
Sounds good, I’ll update this PR later today. (linking back to airbnb/javascript#1834) |
@ljharb, thanks for landing this! Would it be possible to release a new version to the npm registry? |
I’m hoping to do that tonight or tomorrow. |
Released in v7.10.0 |
Looks like it just works.
The peer dep change sees to it that this plugin can be used in projects that use eslint 5 and npm 2 (the default in node 4) without
npm install
failing.