Skip to content
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

Merged
merged 1 commit into from Jun 24, 2018
Merged

Conversation

papandreou
Copy link
Contributor

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.

@papandreou
Copy link
Contributor Author

The test failure also happens on master and is due to babel-eslint@8.2.4. Downgrading to 8.2.3 fixes it.

git bisect says:

e63962ddd340e3a412ee33d9f0ed443a88d3975f is the first bad commit
commit e63962ddd340e3a412ee33d9f0ed443a88d3975f
Author: Cristian Pallarés <cristian@pallares.io>
Date:   Wed Jun 6 00:18:13 2018 +0200

    refactor: rename babylon to @babel/parser

:040000 040000 c6b299854dde266cc8960551ab1ee334fd6f5e93 d08c0f67d7b0d9b795dbb8a8702ca787c3318182 M	lib

Perhaps the easiest fix is to just drop node 4 support now that everyone else in the eslint ecosystem is doing it.

papandreou referenced this pull request in babel/babel-eslint Jun 23, 2018
@existentialism
Copy link

existentialism commented Jun 23, 2018

@papandreou just released babel-eslint@8.2.5, which reverts the babel 7-b51 bump.

Copy link
Member

@ljharb ljharb left a 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.

papandreou added a commit to papandreou/eslint-plugin-react that referenced this pull request Jun 23, 2018
@papandreou
Copy link
Contributor Author

@ljharb, thanks for the quick response!

Please also add testing for eslint 4 vs 5 here: https://github.com/yannickcr/eslint-plugin-react/blob/master/.travis.yml

Okay, do you mean like this? d717068

... I guess the exact configuration depends on your answer to the below question :)

Also, please make the dev dep range for eslint identical to the peer dep range.

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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, fixed in d0bebd7 :)

@ljharb
Copy link
Member

ljharb commented Jun 23, 2018

As for node 4, no, we should not drop support for it (nobody should). I’ll look into the failing tests.

papandreou added a commit to papandreou/eslint-plugin-react that referenced this pull request Jun 23, 2018
@papandreou
Copy link
Contributor Author

@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
Copy link
Member

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

Copy link
Contributor Author

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 :)

@ljharb
Copy link
Member

ljharb commented Jun 23, 2018

ahhh well true, in that case for eslint 5 only, we should definitely not be testing on node 4.

@papandreou
Copy link
Contributor Author

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.
It's probably faster for you to set that up? :)

@ljharb
Copy link
Member

ljharb commented Jun 23, 2018

Sounds good, I’ll update this PR later today.

(linking back to airbnb/javascript#1834)

@ljharb ljharb merged commit c6cc4ab into jsx-eslint:master Jun 24, 2018
@papandreou
Copy link
Contributor Author

@ljharb, thanks for landing this! Would it be possible to release a new version to the npm registry?

@ljharb ljharb deleted the feature/eslint5 branch June 24, 2018 17:08
@ljharb
Copy link
Member

ljharb commented Jun 24, 2018

I’m hoping to do that tonight or tomorrow.

@ljharb
Copy link
Member

ljharb commented Jun 25, 2018

Released in v7.10.0

calebeby pushed a commit to Pigmice2733/scouting-frontend that referenced this pull request Jun 30, 2018
This Pull Request updates dependency [eslint-plugin-react](https://github.com/yannickcr/eslint-plugin-react) from `v7.9.1` to `v7.10.0`



<details>
<summary>Release Notes</summary>

### [`v7.10.0`](https://github.com/yannickcr/eslint-plugin-react/blob/master/CHANGELOG.md#&#8203;7100---2018-06-24)
[Compare Source](jsx-eslint/eslint-plugin-react@v7.9.1...v7.10.0)
##### Added
* Allow eslint ^5 ([#&#8203;1843][] @&#8203;papandreou, @&#8203;ljharb)
* [`no-unsafe`][] rule ([#&#8203;1831][], [#&#8203;1830][] @&#8203;sergei-startsev)
* [`no-will-update-set-state`][]: Account for `UNSAFE_` methods ([#&#8203;1845][], [#&#8203;1844][] @&#8203;alexzherdev)
##### Fixed
* [`no-typos`][]: Fix static propTypes handling ([#&#8203;1827][], [#&#8203;1677][] @&#8203;alexzherdev)
* [`destructuring-assignment`][]: Allow LHS ([#&#8203;1825][], [#&#8203;1728][] @&#8203;alexzherdev)
* [`no-unused-prop-types`][]: Fix crash when encountering mixed union and intersection flow types ([#&#8203;1806][] @&#8203;yannickcr)
##### Changed
* Typo fixes in [`jsx-no-target-blank`][] ([#&#8203;1805][] @&#8203;ferhatelmas))

[#&#8203;1845]: `jsx-eslint/eslint-plugin-react#1845
[#&#8203;1844]: `jsx-eslint/eslint-plugin-react#1844
[#&#8203;1843]: `jsx-eslint/eslint-plugin-react#1843
[#&#8203;1831]: `jsx-eslint/eslint-plugin-react#1831
[#&#8203;1830]: `jsx-eslint/eslint-plugin-react#1830
[#&#8203;1827]: `jsx-eslint/eslint-plugin-react#1827
[#&#8203;1825]: `jsx-eslint/eslint-plugin-react#1825
[#&#8203;1806]: `jsx-eslint/eslint-plugin-react#1806
[#&#8203;1805]: `jsx-eslint/eslint-plugin-react#1805
[#&#8203;1728]: `jsx-eslint/eslint-plugin-react#1728
[#&#8203;1677]: `jsx-eslint/eslint-plugin-react#1677

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants