-
Notifications
You must be signed in to change notification settings - Fork 19
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
Upgrade peer dependency versions #101
Conversation
This finally avoids a warning from eslint-config-react-app
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.
Minor, remove the jsx-a11y/href-no-hash
rule from the config.
"babel-eslint": "^9.0.0", | ||
"chalk": "^2.4.1", | ||
"eslint": "^5.8.0", | ||
"eslint-config-react-app": "^3.0.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.
The 3.x release of eslint-config-react-app/index.js
no longer includes the jsx-a11y/href-no-hash
rule, not sure what release it was removed from, but the following should be removed from HM's config here
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.
Even if it was removed from there, I'd like to separate the upgrade from the decision of whether we should be removing that rule.
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 will address this in a follow-up PR on top of this PR.
Via https://eslint.org/docs/user-guide/migrating-to-5.0.0 https://eslint.org/docs/user-guide/migrating-to-5.0.0#eslint-recommended-changes
Worth noting in the release docs that these new rules are enabled by default. https://eslint.org/docs/user-guide/migrating-to-5.0.0#rule-default-changes
The |
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.
Providing this is well tested, I have no issues with merging this.
It's that time again: time to upgrade our peer dependencies! This does bump the eslint version, so if @joehoyle @rmccue I acknowledge we may want to exercise more caution and hold off for a bit. That said, I'm in favor of testing this and then releasing as
0.6.0
to permit projects to take advantage of the changes. Because of how semver handles <1.0 versions a0.6.0
release won't affect any projects currently using^0.5.0
, making the change strictly opt-in and forward-facing.The other benefit is that there was a conflict between
eslint-config-react-app
andeslint-plugin-jsx-a11y
, which the latest bump to the React App config mercifully resolves.Resolves #56